fix(core/txpool/legacypool): fix flaky TestAllowedTxSize
#31142
Conversation
…ally reowrk the transaction generation logic to change the data content and length until the desired transaction size is reached. This makes the test deterministic regarding transaction sizes, whilst keeping the random transaction data.
| 3 + // 3 is the minimum signature encoding size | ||
| 5 - // 5 is the maximum data length encoding size | ||
| 0 // 0 is the minimum data length encoding size | ||
| for range tries { |
There was a problem hiding this comment.
I think this could be simplified by assuming that we want to find a target signature size of 65 bytes, and then in an infinite loop generate signed data transactions of targetSize - 65 and return if the signed tx size is the target size.
There was a problem hiding this comment.
I'm still working on this, it's taking a bit longer than expected, please keep this opened ~24 hours and I'll get it done. Thanks!
There was a problem hiding this comment.
Done in 7c5f53e although it it is "less dumb", it is also longer and less simple unfortunately. The main reason of this complexity being that:
len(rlp(signedTx)) is not always len(rlp(unsignedTx)) + signatureLength (same signatures lengths, same unsigned transaction)
This must be due to some RLP implementation details, but I would rather not dig too much into that.
Therefore, we could also revert it to be less-clever/simpler as it was before, kind of (gently) bruteforcing until we have the desired transaction size. Any other suggestion is more than welcome though!
There was a problem hiding this comment.
Thanks for taking the time to make these changes. I think I'm only now realizing that creating a generic sizedDataTransaction function is pretty non-trivial.
Given that the test-case is really concerned about the boundary conditions (maxSize - 1, maxSize, maxSize + 1), I'm wondering if we could pair down the complexity of this test by only generating txs of these sizes.
It's not quite as nice as generating a wider variety of inputs, but it could probably suffice here.
There was a problem hiding this comment.
If we have a fixed key and deterministic data generation (no crand.Read), there are two ways to do this:
- have fully pre-determined transactions, for example as hex encoding of RLP encoding. This removes a lot of logic, but makes the source code rather big given transactions are rather big in sizes. We could move these to a testdata directory. Thoughts?
- Keep generating with a bit less logic, given the signature length can vary with the data length, even if the key is fixed and the data is all zeroes. This is what I've done in the last commit 399949c
- Add check for a target size greater or equal to minimum size - Generate transactions only with 65 byte signatures - Adjust data length when signed transaction RLP encoding size varies by +- 1,2 bytes
|
This seems like a very very complex solution to the problem. I would like to discuss this on triage |
- Same ECDSA key - Deterministic generation of data
| // We use a deterministic ecdsa key to obtain signed transactions of pre-determined sizes. | ||
| key := &ecdsa.PrivateKey{ | ||
| PublicKey: ecdsa.PublicKey{ | ||
| Curve: elliptic.P256(), | ||
| X: new(big.Int).SetBytes(common.Hex2Bytes("78e68ff485cc934e4096f24091931e91334d6841c6c2be93d1a930e59783902d")), | ||
| Y: new(big.Int).SetBytes(common.Hex2Bytes("ca8db229299a4f937eeb0a36c451751cd59df978aceaaf190c28783c321cf267")), | ||
| }, | ||
| D: new(big.Int).SetBytes(common.Hex2Bytes("e521e757356c1f197f704502dfa10333993e3705ca3d304d6a349461ed67ae71")), | ||
| } |
There was a problem hiding this comment.
This statement can be replaced by crypto.HexToECDSA("e521e757356c1f197f704502dfa10333993e3705ca3d304d6a349461ed67ae71")
There was a problem hiding this comment.
Done, thanks for the tip 👍
Any update or idea on how to make this test simpler? 🤔 |
|
Closing because we decided to include the simpler alternative #31836 |
First of all, my apologies for not correctly/fully fixing this in my previous PR #30975
The test was failing occasionally because:
To work around this, the logic to generate transaction with a target size was reworked: the transaction data content and length is changed until the desired transaction size is reached. This makes the test deterministic regarding transaction sizes.