Add new nonce-fix and replace mpool tools#6911
Closed
sudo-shashank wants to merge 16 commits into
Closed
Conversation
Contributor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sudo-shashank
force-pushed
the
shashank/nonce-fix-cmd
branch
from
078a644 to
6c3da7c
Compare
sudo-shashank
force-pushed
the
shashank/nonce-fix-cmd
branch
from
6c3da7c to
1f52ab9
Compare
sudo-shashank
force-pushed
the
shashank/nonce-fix-cmd
branch
from
1f52ab9 to
f7f12ff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/rpc/methods/gas.rs (1)
334-334: ⚡ Quick winPrefer
pub(crate)forcap_gas_feeunless external crate consumers are intentional.This appears to be an internal utility used across crate modules;
pubbroadens API surface and long-term compatibility burden.🔧 Suggested change
-pub fn cap_gas_fee( +pub(crate) fn cap_gas_fee(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/methods/gas.rs` at line 334, Change the visibility of the cap_gas_fee function from public to crate-only to limit the API surface: locate the function named cap_gas_fee and replace its declaration visibility from pub to pub(crate) so it remains accessible across crate modules but not exported to dependent crates; ensure there are no external call sites expecting a public symbol and run tests to verify internal usages still compile.src/message_pool/mod.rs (1)
17-17: ⚡ Quick winNarrow
utilsvisibility to crate scope instead of public wildcard export.
pub use utils::*exposes internal helpers as part of external API. For this PR’s usage,pub(crate)access is sufficient and safer for API stability.As per coding guidelines, "`**/mod.rs`: Each module should have a `mod.rs` file with public API exports and private submodules for implementation details".🔧 Suggested change
-pub use self::{ +pub(crate) use self::{ config::*, errors::*, mpool_locker::MpoolLocker, msgpool::{msg_pool::MessagePool, *}, nonce_tracker::NonceTracker, utils::*, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/message_pool/mod.rs` at line 17, The file exposes internal helpers via a public wildcard re-export (utils), so change the export from a public API to crate-scoped by replacing the `pub use utils::*` export with a crate-only export (e.g., `pub(crate) use utils::*`) in src/message_pool/mod.rs so the utils module remains internal; update the mod's export line and run cargo check to ensure no external code relied on the now-hidden symbols and adjust any caller imports if needed.src/cli/subcommands/mpool_cmd.rs (1)
977-977: ⚡ Quick winUse
.get(0)instead of direct indexing in tests.
pending[0]can panic if fixture setup changes;.get(0)keeps the assertion failure explicit and follows repo guidance.As per coding guidelines, "`**/*.rs`: Use `.get()` instead of direct indexing `[index]` to avoid panics".🔧 Suggested change
- assert_eq!(found.cid(), pending[0].cid()); + assert_eq!( + found.cid(), + pending.get(0).expect("expected one pending message").cid() + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/subcommands/mpool_cmd.rs` at line 977, The test currently indexes pending with pending[0] which can panic; change the assertion to use pending.get(0) and fail explicitly (e.g., pending.get(0).expect("expected at least one pending entry")) when obtaining the element before comparing CIDs so the assertion is explicit and non-panicking; update the line with the symbols found.cid(), pending.get(0).expect(...).cid() inside the existing assert_eq! call.tests/calibnet_mpool_tools.rs (1)
60-63: ⚡ Quick winThis assertion does not verify replacement behavior.
Polling
StateSearchMsgfor the original CID can succeed even if replacement never happened (the original message may simply be mined). Consider asserting on the new CID returned bympool replace.🔧 Suggested direction
- forest_cli(&[ + let out = forest_cli(&[ "mpool", "replace", "--from", addr, "--nonce", &nonce.to_string(), "--auto", - ]) - .unwrap(); + ]).unwrap(); + + let new_cid = out + .strip_prefix("new message cid: ") + .context("unexpected mpool replace output")? + .trim() + .to_string(); assert!( - poll_until_state_search_msg(&cid).await.is_ok(), - "mpool replace --auto should replace message {cid} from {addr} at nonce {nonce}." + poll_until_state_search_msg(&new_cid).await.is_ok(), + "mpool replace --auto should land replacement message {new_cid} for nonce {nonce}." );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/calibnet_mpool_tools.rs` around lines 60 - 63, The test currently only polls for the original CID using poll_until_state_search_msg(&cid) which can succeed even if replacement never occurred; change the test to capture the new CID returned by the `mpool replace --auto` command (the replacement result), then assert that poll_until_state_search_msg(&new_cid).await.is_ok() to verify the replacement was registered, and optionally assert that the original cid is not pending/mined in the mempool if you can query mempool state; locate the `mpool replace` invocation and the poll_until_state_search_msg call in tests/calibnet_mpool_tools.rs and replace the existing assertion to use the returned new CID instead of the original `cid`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/subcommands/mpool_cmd.rs`:
- Around line 223-225: Add a validation in the manual replace path to reject
cases where gas_feecap is less than gas_premium: after computing gas_premium and
gas_feecap (the lines that set original_msg.gas_premium and
original_msg.gas_fee_cap) check if gas_feecap < gas_premium and return a CLI
validation error (with a clear message like "gas-feecap must be >= gas-premium")
instead of proceeding to set fields or pushing; update the same branch that
contains the if let Some(limit) = gas_limit block so the check runs before
modifying original_msg or attempting the push.
In `@tests/calibnet_mpool_tools.rs`:
- Around line 14-15: These calibnet mpool integration tests mutate shared state
and must run serially; add the serial_test::serial attribute to them. For the
test function mpool_nonce_fix_auto_unblocks_pending (and the other calibnet
mpool test in this file), annotate each test with #[serial_test::serial] in
addition to #[tokio::test] (order doesn't matter) so the tests run sequentially
and avoid nonce/mempool race flakiness.
---
Nitpick comments:
In `@src/cli/subcommands/mpool_cmd.rs`:
- Line 977: The test currently indexes pending with pending[0] which can panic;
change the assertion to use pending.get(0) and fail explicitly (e.g.,
pending.get(0).expect("expected at least one pending entry")) when obtaining the
element before comparing CIDs so the assertion is explicit and non-panicking;
update the line with the symbols found.cid(), pending.get(0).expect(...).cid()
inside the existing assert_eq! call.
In `@src/message_pool/mod.rs`:
- Line 17: The file exposes internal helpers via a public wildcard re-export
(utils), so change the export from a public API to crate-scoped by replacing the
`pub use utils::*` export with a crate-only export (e.g., `pub(crate) use
utils::*`) in src/message_pool/mod.rs so the utils module remains internal;
update the mod's export line and run cargo check to ensure no external code
relied on the now-hidden symbols and adjust any caller imports if needed.
In `@src/rpc/methods/gas.rs`:
- Line 334: Change the visibility of the cap_gas_fee function from public to
crate-only to limit the API surface: locate the function named cap_gas_fee and
replace its declaration visibility from pub to pub(crate) so it remains
accessible across crate modules but not exported to dependent crates; ensure
there are no external call sites expecting a public symbol and run tests to
verify internal usages still compile.
In `@tests/calibnet_mpool_tools.rs`:
- Around line 60-63: The test currently only polls for the original CID using
poll_until_state_search_msg(&cid) which can succeed even if replacement never
occurred; change the test to capture the new CID returned by the `mpool replace
--auto` command (the replacement result), then assert that
poll_until_state_search_msg(&new_cid).await.is_ok() to verify the replacement
was registered, and optionally assert that the original cid is not pending/mined
in the mempool if you can query mempool state; locate the `mpool replace`
invocation and the poll_until_state_search_msg call in
tests/calibnet_mpool_tools.rs and replace the existing assertion to use the
returned new CID instead of the original `cid`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e1af9eac-68cc-4b79-abe2-5c8263471529
📒 Files selected for processing (12)
.config/forest.dicCargo.tomldocs/docs/users/reference/cli.mdmise.tomlsrc/cli/subcommands/mpool_cmd.rssrc/message_pool/mod.rssrc/message_pool/msgpool/msg_set.rssrc/message_pool/msgpool/utils.rssrc/rpc/methods/gas.rstests/calibnet_mpool_tools.rstests/calibnet_wallet.rstests/common/calibnet_wallet_helpers.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/subcommands/mpool_cmd.rs (1)
478-493: 💤 Low valueConsider making
gas_premiumconfigurable for filler messages.The hardcoded
gas_premiumof 5 atto works for typical conditions but may cause fillers to be stuck during network congestion. Users can overridegas_fee_capbut notgas_premium. This is consistent with Lotus's lotus-shed nonce-fix, so acceptable as-is—flagging as optional enhancement for future consideration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/subcommands/mpool_cmd.rs` around lines 478 - 493, The filler messages currently hardcode gas_premium to TokenAmount::from_atto(5u64) inside the loop that builds Message (see Message { ... gas_premium: TokenAmount::from_atto(5u64), ... }) before WalletSignMessage::call and MpoolPush::call; make gas_premium configurable by adding a CLI/command option (e.g., --filler-gas-premium) or a parameter passed into this command, parse it into a TokenAmount variable (e.g., filler_gas_premium) and replace the hardcoded TokenAmount::from_atto(5u64) with that variable so filler messages use the user-provided gas_premium value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cli/subcommands/mpool_cmd.rs`:
- Around line 478-493: The filler messages currently hardcode gas_premium to
TokenAmount::from_atto(5u64) inside the loop that builds Message (see Message {
... gas_premium: TokenAmount::from_atto(5u64), ... }) before
WalletSignMessage::call and MpoolPush::call; make gas_premium configurable by
adding a CLI/command option (e.g., --filler-gas-premium) or a parameter passed
into this command, parse it into a TokenAmount variable (e.g.,
filler_gas_premium) and replace the hardcoded TokenAmount::from_atto(5u64) with
that variable so filler messages use the user-provided gas_premium value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37e3bdb4-ab2f-41e9-9a08-6fb9324a816b
📒 Files selected for processing (6)
docs/docs/users/reference/cli.mdsrc/cli/subcommands/mpool_cmd.rssrc/message_pool/mod.rssrc/message_pool/msgpool/utils.rssrc/rpc/methods/gas.rstests/calibnet_mpool_tools.rs
✅ Files skipped from review due to trivial changes (1)
- docs/docs/users/reference/cli.md
sudo-shashank
changed the title
nonce-fix and replace mpool tools
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 of changes
Changes introduced in this pull request:
nonce-fixandreplacecli commands.Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
New Features
forest-cli mpool nonce-fixcommand to fill nonce gaps with address range and gas fee customization options.forest-cli mpool replacecommand to replace pending messages with adjusted gas parameters using replace-by-fee mechanism, supporting both auto and manual modes.Tests