You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(cmd/devp2p): price fixture txs at the chain gas floor, close #3062 - #3065
generateTxs priced every fixture batch at 1 wei. The XDC txpool validates against params.GetMinGasPrice(pendingNumber, config) (core/txpool/validation.go:307), so on the hydrated gas2500x fixture the whole 2000-tx batch was rejected with under min gas price. The live-devnet branch of the same test had a hardcoded DefaultMinGasPrice*2500 multiplier; the fixture branch was never adapted. Both branches now derive the price from the chain's own fork schedule at the pending block height, the exact source the node uses, and generateTxs refuses to return a batch priced below that floor. The upstream 2000-tx calibration is unchanged in this PR; changing it is tracked in #3067 (see the residual note below).
Files:cmd/devp2p/internal/ethtest/transaction_suite.go (helper, pricing, assertion, matrix branch) and transaction_suite_test.go (two-tier regression + pending-height boundary case). Suite only: no node, txpool, protocol, config, or dependency changes. No generated-code changes.
Evidence
Before:generateTxs used big.NewInt(1); the frozen devnet node logged under min gas price for the fixture batch and LargeTxRequest_Xdc166 reported missing 2000 txs.
After (chain-level A/B, silent-phase devnet, two independent rounds): with a devp2p built from dev-upgrade, each round logs under min gas price = 1; with a devp2p built from this branch, under min gas price = 0 in both rounds. The price-rejection path is gone. In matrix mode (48 txs) both binaries return ok 1 LargeTxRequest_Xdc166, so unifying the two branches does not regress the live-devnet path.
Unit tests:TestGenerateTxsClearsChainGasFloor covers the 1x (testdata) and 2500x (testdata/xdc) fixtures and asserts every tx equals the derived floor, with sequential nonces and matching hashes; TestChainGasPriceFloorUsesPendingNumber pins the head+1 rule at a fork boundary (601 -> 2500x, 602 -> 1x). go test ./cmd/devp2p/internal/ethtest/... -count=1 = ok 18.184s.
Mutations (3/3 KILLED): reverting floor to big.NewInt(1), dropping + common.Big1, and pinning the 2500x tier while ignoring the chain config each fail the new tests; probes were removed and the restored tree passes.
Full per-commit chain: 1/1 PASS, make all, make generate (No stale files detected.), make tidy (go mod tidy -diff clean), clean-tree gate, make lint (0 issues.), go vet ./..., make test all green. gofmt -s/goimports/git diff --check clean.
Residual, not fixed here (please note before merging): the fixture-mode 2000-tx LargeTxRequest_Xdc166 still reports not ok on the live devnet, but the node now rejects with nonce too high (~255 per round), not under min gas price — the pricing gate from cmd/devp2p: fixture-mode generateTxs prices at 1 wei, below the hydrated pool floor #3062 is gone. This is a separate suite defect. The corrected mechanism: nonce too high is raised during admission at core/txpool/validation.go:234 (PendingNonce + common.LimitThresholdNonceInQueue=10 < tx.Nonce()) and :241 (gapped nonce), so those txs never enter the queue; AccountQueue=64 (core/txpool/legacypool/queue.go:199) only trims the few that do. The real blockers are the node's maxTxRetrievals=256 batched requests (eth/fetcher/tx_fetcher.go:48, with f.announces iterated in map order so the first batch's nonces are unordered) and the suite returning after serving only the first GetPooledTransactionsMsg request (transaction_suite.go:474), leaving at least 1744 txs never delivered. The ~255 count matches the 256-hash batch, not the 64 queue cap. The 2000-tx calibration is now allowed to change and the fix is tracked in cmd/devp2p: fixture-mode LargeTxRequest never delivers all 2000 txs (batched retrieval + one-shot response) #3067, so it is out of scope for this PR. This PR deliberately does not claim that the 2000-tx case turns green.
Blast Radius: suite-only. No node, consensus, txpool, or wire-format behaviour changes; only the prices of transactions generated by devp2p rlpx eth-test change. CI is unaffected except for the two new local tests. The one observable consequence is that suite-generated txs now cost gas at the chain floor rather than 1 wei, which is the intended behaviour.
The ethtest fixture batch was priced at 1 wei while the node enforces a chain-derived minimum gas price (2500x on the hydrated gas2500x fixture). Every generated transaction was silently dropped, so the pooled-transaction conformance case could not pass. This derives both the fixture and live-devnet batches from the same params.GetMinGasPrice schedule the node uses, asserts the built batch clears the floor, and adds local regression tests pinning the 1x/2500x tiers and the pending-block rule.
Types of changes
What types of changes does your code introduce to XDC network? Put an ✅ in the boxes that apply
build: Changes that affect the build system or external dependencies
ci: Changes to CI configuration files and scripts
chore: Changes that don't change source code or tests
docs: Documentation only changes
feat: A new feature
fix: A bug fix
perf: A code change that improves performance
refactor: A code change that neither fixes a bug nor adds a feature
revert: Revert something
style: Changes that do not affect the meaning of the code
test: Adding missing tests or correcting existing tests
Impacted Components
Which parts of the codebase does this PR touch? Put an ✅ in the boxes that apply
Consensus
Account
Network
Geth
Smart Contract
External components
Not sure (Please specify below)
Only cmd/devp2p/internal/ethtest, the RLPx/devp2p conformance test harness. No node or protocol code is touched.
Checklist
Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that
This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
Tested on a private network from the genesis block and monitored the chain operating correctly for multiple epochs.
Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
Tested the backwards compatibility.
Tested with XDC nodes running this version co-exist with those running the previous version.
Relevant documentation has been updated as part of this PR
N/A
Reasons for unchecked items:
Private-network multi-epoch soak: not applicable. This changes only the devp2p test harness, not chain operation. A four-node fixture devnet was brought up from genesis (frozen suite target plus three validators for QC quorum) to run the live A/B comparison; it was a conformance-suite run, not a chain-stability soak.
End-to-end plan: build devp2p from this branch, start the fixture devnet (Local_DPoS_Setup/run-ethtest-fixture.sh), then run devp2p rlpx eth-test --node <frozen.enode> --chain cmd/devp2p/internal/ethtest/testdata/xdc --tap --run '^LargeTxRequest_Xdc166$'; set MATRIX_SENDER_KEY and MATRIX_SENDER_NONCE to exercise the 48-tx live-devnet branch. The Evidence section records the baseline-vs-fixed node-log comparison.
Backwards compatibility: verified at the harness level. No persisted data, config, API, or wire format changes; the only behaviour change is the gas price of suite-generated transactions.
Mixed-version node coexistence: not applicable, no node binary or protocol change.
Documentation: not applicable. The obsolete hardcoded-multiplier comment was removed with the code it described; no user-facing docs change.
N/A is not ticked because the applicable items are individually addressed above.
Summary by CodeRabbit
Bug Fixes
Transaction test scenarios now use the chain’s minimum gas price, avoiding transactions that are priced below the configured floor.
Tests
Added coverage for gas-price floors across fork boundaries and for generated transaction counts, nonces, and hash entries.
…inOrg#3062
The fixture batch was priced at 1 wei while the hydrated chain enforces a
gas2500x floor of 2500 x DefaultMinGasPrice. All 2000 transactions were
silently dropped by the txpool, so LargeTxRequest_Xdc166 failed with every
announced hash missing even though the node behaved correctly.
Derive the price from the chain's own fork schedule at the pending block
height, the same source the node uses in txpool validation, and assert the
built batch clears that floor before it is announced. The live-devnet branch
uses the same derivation instead of a hardcoded 2500 multiplier. Local tests
pin the 1x and 2500x tiers and the pending-height boundary so a hardcoded
price cannot silently return.
CloseXinFinOrg#3062
Ref: XinFinOrg#2968
No actionable comments were generated in the recent review.
Walkthrough
Matrix-mode and fixture-generated transactions now use the chain’s minimum gas price for the next block. The generator rejects signed transactions below that floor. Tests cover pending-block floor selection and generated transaction batches.
Changes: Transaction gas-price floor
Layer / File(s)
Summary
Calculate and apply the chain floor cmd/devp2p/internal/ethtest/transaction_suite.go, cmd/devp2p/internal/ethtest/transaction_suite_test.go
A helper calculates the minimum gas price using the next block number. Matrix-mode and generated transactions use that floor, and the generator rejects signed transactions below it. Tests verify floor selection, batch size, gas prices, nonces, and hash-map entries.
Issue 2968: The PR changes fixture-generated transaction pricing for the calibrated batch, addressing part of the issue’s remaining fixture-world scope.
Merge Risk: Minimal · up to aca32
This change only adjusts how test transactions are priced in the devp2p ethtest suite. It has no production impact, and no merge-blocking risk was found.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: fixture transactions in the devp2p eth-test suite are now priced at the chain gas floor, and it references the closed issue #3062. It is specific and matches the chang…
Linked Issues check
Issue #3062 has three coding acceptance criteria. (1) Derive the price from the chain's fork schedule through params.GetMinGasPrice: met. The diff adds chainGasPriceFloor(), which computes head+1 and …
Out of Scope Changes check
Whole-PR diff from merge base 4001230 to head aca3219 covers cmd/devp2p/internal/ethtest/transaction_suite.go (import of params, chainGasPriceFloor helper, matrix-branch pricing, fixture-branch pric…
Description check
The description follows the repository template closely. It has a clear summary, the Types of changes section (fix and test checked), Impacted Components (Network checked, with a scope note), and a ch…
Full details: Title check
Explanation
The title names the main change: fixture transactions in the devp2p eth-test suite are now priced at the chain gas floor, and it references the closed issue #3062. It is specific and matches the changeset, which derives transaction prices from params.GetMinGasPrice and adds tests for the floor.
Full details: Linked Issues check
Explanation
Issue #3062 has three coding acceptance criteria. (1) Derive the price from the chain's fork schedule through params.GetMinGasPrice: met. The diff adds chainGasPriceFloor(), which computes head+1 and calls params.GetMinGasPrice(pending, s.chain.config). generateTxs now uses this floor instead of big.NewInt(1). (2) Keep the 2000-transaction calibration: met. The diff does not change the numTxs value or the caller. (3) Assert the built batch clears the chain floor before announcement: met. generateTxs returns an error when signed.GasPriceIntCmp(floor) < 0. The matrix branch in TestLargeTxRequest_Xdc166 also uses the same derived floor instead of the hardcoded DefaultMinGasPrice*2500 multiplier, which the issue names as the reference. Regression tests are reported in transaction_suite_test.go (TestGenerateTxsClearsChainGasFloor, TestChainGasPriceFloorUsesPendingNumber). I read the suite diff but did not open the test file, so test behaviour rests on the PR summary. The PR states that the fixture-mode 2000-tx case still fails on the live devnet with nonce too high. The issue does not list that outcome as a coding acceptance criterion. The devnet A/B logs are reported evidence and were not independently verified.
Full details: Out of Scope Changes check
Explanation
Whole-PR diff from merge base 4001230 to head aca3219 covers cmd/devp2p/internal/ethtest/transaction_suite.go (import of params, chainGasPriceFloor helper, matrix-branch pricing, fixture-branch pricing, floor assertion) and transaction_suite_test.go (regression tests). Every hunk in transaction_suite.go maps to issue #3062. The PR description states that no node, txpool, protocol, config, dependency, or generated-code files change. The ignored_files list is empty. The test file change is supporting test coverage for the issue. No unrelated change was found in the inspected diff.
Full details: Description check
Explanation
The description follows the repository template closely. It has a clear summary, the Types of changes section (fix and test checked), Impacted Components (Network checked, with a scope note), and a checklist with justified reasons for unchecked items. It explains the root cause (fixture batch priced at 1 wei below the chain gas floor), the fix (derive the floor via params.GetMinGasPrice at the pending height, assert the batch clears it), test coverage, and an end-to-end test plan. It also records a known residual: the fixture-mode 2000-tx case still fails with nonce too high, deferred to #3067. Minor gaps: the 'Tested on a private network from the genesis block' box is unchecked with a reason given, and the Documentation item is marked not applicable with a reason. The reasons are provided, so the description is complete.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
No code changes were needed here: the automated review produced no actionable comments and there are no open review threads on this PR. The fixture-mode 2000-tx delivery gap is tracked separately in #3067 and fixed in PR #3068; this PR stays scoped to the chain gas floor. Validation for the delivery work is reported on #3068.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
generateTxspriced every fixture batch at 1 wei. The XDC txpool validates againstparams.GetMinGasPrice(pendingNumber, config)(core/txpool/validation.go:307), so on the hydratedgas2500xfixture the whole 2000-tx batch was rejected withunder min gas price. The live-devnet branch of the same test had a hardcodedDefaultMinGasPrice*2500multiplier; the fixture branch was never adapted. Both branches now derive the price from the chain's own fork schedule at the pending block height, the exact source the node uses, andgenerateTxsrefuses to return a batch priced below that floor. The upstream 2000-tx calibration is unchanged in this PR; changing it is tracked in #3067 (see the residual note below).Files:
cmd/devp2p/internal/ethtest/transaction_suite.go(helper, pricing, assertion, matrix branch) andtransaction_suite_test.go(two-tier regression + pending-height boundary case). Suite only: no node, txpool, protocol, config, or dependency changes. No generated-code changes.Evidence
generateTxsusedbig.NewInt(1); the frozen devnet node loggedunder min gas pricefor the fixture batch andLargeTxRequest_Xdc166reportedmissing 2000 txs.devp2pbuilt fromdev-upgrade, each round logsunder min gas price = 1; with adevp2pbuilt from this branch,under min gas price = 0in both rounds. The price-rejection path is gone. In matrix mode (48 txs) both binaries returnok 1 LargeTxRequest_Xdc166, so unifying the two branches does not regress the live-devnet path.TestGenerateTxsClearsChainGasFloorcovers the 1x (testdata) and 2500x (testdata/xdc) fixtures and asserts every tx equals the derived floor, with sequential nonces and matching hashes;TestChainGasPriceFloorUsesPendingNumberpins thehead+1rule at a fork boundary (601 -> 2500x, 602 -> 1x).go test ./cmd/devp2p/internal/ethtest/... -count=1= ok 18.184s.floortobig.NewInt(1), dropping+ common.Big1, and pinning the 2500x tier while ignoring the chain config each fail the new tests; probes were removed and the restored tree passes.make all,make generate(No stale files detected.),make tidy(go mod tidy -diffclean), clean-tree gate,make lint(0 issues.),go vet ./...,make testall green.gofmt -s/goimports/git diff --checkclean.LargeTxRequest_Xdc166still reportsnot okon the live devnet, but the node now rejects withnonce too high(~255 per round), notunder min gas price— the pricing gate from cmd/devp2p: fixture-mode generateTxs prices at 1 wei, below the hydrated pool floor #3062 is gone. This is a separate suite defect. The corrected mechanism:nonce too highis raised during admission atcore/txpool/validation.go:234(PendingNonce + common.LimitThresholdNonceInQueue=10 < tx.Nonce()) and:241(gapped nonce), so those txs never enter the queue;AccountQueue=64(core/txpool/legacypool/queue.go:199) only trims the few that do. The real blockers are the node'smaxTxRetrievals=256batched requests (eth/fetcher/tx_fetcher.go:48, withf.announcesiterated in map order so the first batch's nonces are unordered) and the suite returning after serving only the firstGetPooledTransactionsMsgrequest (transaction_suite.go:474), leaving at least 1744 txs never delivered. The~255count matches the 256-hash batch, not the 64 queue cap. The 2000-tx calibration is now allowed to change and the fix is tracked in cmd/devp2p: fixture-mode LargeTxRequest never delivers all 2000 txs (batched retrieval + one-shot response) #3067, so it is out of scope for this PR. This PR deliberately does not claim that the 2000-tx case turns green.Merge Danger
Door: two-way. Test-harness-only change, trivially revertable.
Blast Radius: suite-only. No node, consensus, txpool, or wire-format behaviour changes; only the prices of transactions generated by
devp2p rlpx eth-testchange. CI is unaffected except for the two new local tests. The one observable consequence is that suite-generated txs now cost gas at the chain floor rather than 1 wei, which is the intended behaviour.Close #3062
Ref: #2968
Proposed changes
The ethtest fixture batch was priced at 1 wei while the node enforces a chain-derived minimum gas price (2500x on the hydrated gas2500x fixture). Every generated transaction was silently dropped, so the pooled-transaction conformance case could not pass. This derives both the fixture and live-devnet batches from the same
params.GetMinGasPriceschedule the node uses, asserts the built batch clears the floor, and adds local regression tests pinning the 1x/2500x tiers and the pending-block rule.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 applyOnly
cmd/devp2p/internal/ethtest, the RLPx/devp2p conformance test harness. No node or protocol code is touched.Checklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) thatReasons for unchecked items:
devp2ptest harness, not chain operation. A four-node fixture devnet was brought up from genesis (frozen suite target plus three validators for QC quorum) to run the live A/B comparison; it was a conformance-suite run, not a chain-stability soak.devp2pfrom this branch, start the fixture devnet (Local_DPoS_Setup/run-ethtest-fixture.sh), then rundevp2p rlpx eth-test --node <frozen.enode> --chain cmd/devp2p/internal/ethtest/testdata/xdc --tap --run '^LargeTxRequest_Xdc166$'; setMATRIX_SENDER_KEYandMATRIX_SENDER_NONCEto exercise the 48-tx live-devnet branch. The Evidence section records the baseline-vs-fixed node-log comparison.N/Ais not ticked because the applicable items are individually addressed above.Summary by CodeRabbit