fix(core/txpool/legacypool): fix flaky test TestAllowedTxSize #30975#2172
fix(core/txpool/legacypool): fix flaky test TestAllowedTxSize #30975#2172gzliudan wants to merge 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR targets a flaky unit test in the legacy txpool by making TestAllowedTxSize derive transaction sizes and funding parameters based on the current chain rules, instead of relying on fixed assumptions.
Changes:
- Funds the sender account based on
GetMinGasPrice(head.Number)andhead.GasLimitinstead of a fixed balance. - Computes the maximum allowed transaction data length empirically using actual signed tx sizes, then tests acceptance/rejection around the boundary.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| txWithLargeData := pricedDataTransaction(0, pool.currentHead.Load().GasLimit, minGasPrice, key, largeDataLength) | ||
| maxTxLengthWithoutData := txWithLargeData.Size() - largeDataLength | ||
| maxTxDataLength := txMaxSize - maxTxLengthWithoutData | ||
| for tx := pricedDataTransaction(0, pool.currentHead.Load().GasLimit, minGasPrice, key, maxTxDataLength); tx.Size() > txMaxSize; { | ||
| maxTxDataLength-- | ||
| tx = pricedDataTransaction(0, pool.currentHead.Load().GasLimit, minGasPrice, key, maxTxDataLength) | ||
| } | ||
| minOversizedDataLength := maxTxDataLength + 1 | ||
| for tx := pricedDataTransaction(0, pool.currentHead.Load().GasLimit, minGasPrice, key, minOversizedDataLength); tx.Size() <= txMaxSize; { | ||
| minOversizedDataLength++ | ||
| tx = pricedDataTransaction(0, pool.currentHead.Load().GasLimit, minGasPrice, key, minOversizedDataLength) | ||
| } | ||
|
|
||
| // Try adding a transaction with maximal allowed size | ||
| tx := pricedDataTransaction(0, pool.currentHead.Load().GasLimit, big.NewInt(300000000), key, dataSize) | ||
| tx := pricedDataTransaction(0, pool.currentHead.Load().GasLimit, minGasPrice, key, maxTxDataLength) | ||
| if err := pool.addRemoteSync(tx); err != nil { | ||
| t.Fatalf("failed to add transaction of size %d, close to maximal: %v", int(tx.Size()), err) | ||
| } | ||
| // Try adding a transaction with random allowed size | ||
| if err := pool.addRemoteSync(pricedDataTransaction(1, pool.currentHead.Load().GasLimit, big.NewInt(300000000), key, uint64(rand.Intn(int(dataSize))))); err != nil { | ||
| if err := pool.addRemoteSync(pricedDataTransaction(1, pool.currentHead.Load().GasLimit, minGasPrice, key, uint64(rand.Intn(int(maxTxDataLength+1))))); err != nil { | ||
| t.Fatalf("failed to add transaction of random allowed size: %v", err) |
There was a problem hiding this comment.
maxTxDataLength/minOversizedDataLength are derived using pricedDataTransaction calls that generate fresh random payloads (and thus different signatures) on each iteration. Later you create new transactions again using those lengths (e.g. at the “Try adding…” calls), which can change the RLP-encoded size by a few bytes and reintroduce flakiness around the txMaxSize boundary. To make this deterministic, either (a) build deterministic tx data for this test (fixed byte pattern), or (b) keep/reuse the exact tx instances found by the loops, and for the “random allowed size” case ensure the generated tx is checked/adjusted to be <= txMaxSize before calling addRemoteSync.
|
|
||
| // Find the maximum data length for the kind of transaction which will | ||
| // be generated in the pool.addRemoteSync calls below. | ||
| const largeDataLength = txMaxSize - 200 // enough to have a 5 bytes RLP encoding of the data length number |
There was a problem hiding this comment.
The inline comment about “enough to have a 5 bytes RLP encoding of the data length number” looks inaccurate for the sizes involved here (txMaxSize is 64KB, so the RLP length-of-length for the data field is much smaller). Consider rewording this to avoid implying a specific RLP encoding size, or explain the actual threshold being targeted by the 200-byte margin.
| const largeDataLength = txMaxSize - 200 // enough to have a 5 bytes RLP encoding of the data length number | |
| const largeDataLength = txMaxSize - 200 // leave enough room for RLP overhead and other transaction fields |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
Proposed changes
Ref: ethereum#30975
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that