algorand / algorand-sdk-testing

Testing framework for Algorand SDKs
MIT License
20 stars 35 forks source link

SDK fee per byte tests underestimate txn fee #242

Closed FrankSzendzielarz closed 2 years ago

FrankSzendzielarz commented 2 years ago

The Sign multisig test in offline.feature happens to be the only transaction related unit test that uses fee per byte fee calculation and attempts to test the calculated fee.

The test compares the generated, signed, encoded transaction against the "golden"

The different tests and sdks do this by setting the transaction fee field first to small values. The java implementation sets it to 1 microalgo. The go sdk sets it to fee per byte, in this case 4.

Then the total fee is calculated based on the size of the transaction object.

The calculated fee for the transaction in the test case, 1176, must then be added to the fee field.

However, the transaction size changes as a result and the correct fee should be 1184.

There are 2 issues then here:

  1. The test case has incorrect test data. Field should be fee 1184
  2. The SDKs incorrectly calculate transaction fee, meaning that during network congestion transactions will be rejected even fees above 1000 are paid.
winder commented 2 years ago

Hi Frank,

Sorry for the confusion here. The only requirement for fee values is that the sum of fees in a tx group (one or more txns) is at least 1000 micro algos per transaction. If a transaction is sent with 1176 rather than 1184, it would not be rejected.

If we were to be even more pedantic about it, the fee should probably include the size of the signature -- multisig's can be very large.

There are some cases, like this one, where even our own SDKs went through some hoops to land at a consistent value. Would it be possible to do something similar in yours?

One thing to keep in mind is that the exponential growth of the suggested fee in Algod is probably a bit rash. When we start getting real network congestion, that may be something that needs to be revisited, and there would probably be implications to how the SDKs handle the default fee computations.

FrankSzendzielarz commented 2 years ago

Sorry i don't understand the above. I thought the minimum fee is 1000 and when there is network congestion the fee per byte raises and that the fee may then become over 1000. What happens to the transaction that has insufficient fee? Is it not rejected. Also pls clarify the comment regarding sdks through hoops. I may have missed the code you are referring to.

winder commented 2 years ago

The "fee per byte" is a suggestion, not a requirement. A node considers the txn fee when the txn pool is full, at which point it favors a higher fee.

By jumping through hoops, I was referring to how the need to have a small number in the fee when applying the fee-per-byte. I vaguely remember running into something similar with the Java SDK and needing to initialize the value to something other than 1000 in order for the cucumber test to compute the same value as the others.

FrankSzendzielarz commented 2 years ago

Wow. Ok so the fee discrepancy is inconsequential? Yes i have the test passing but only if the fee is set to a small value in the firet place, as the other tests and sdks do.

winder commented 2 years ago

The exact value is pretty inconsequential, yes.

winder commented 2 years ago

Sounds like this is resolved, let me know if you have any other questions.