Run wallet and mpool tool test on devnet#7263
Open
sudo-shashank wants to merge 18 commits into
Open
Conversation
Contributor
WalkthroughSplits the Calibnet-only dev test CLI into a shared, chain-agnostic ChangesDevnet wallet/mpool test infrastructure
Message search receipt handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Mise as mise test:wallet-devnet
participant Harness as wallet_harness.sh
participant ForestDev as forest-dev tests_cmd
participant Devnet as Devnet (forest container)
CI->>Mise: run test:wallet-devnet
Mise->>Devnet: setup.sh / check.sh
Mise->>Harness: source and devnet_wallet_env_init
Harness->>Devnet: read token, derive genesis address
Harness->>Devnet: fund dedicated test wallet
Mise->>ForestDev: run devnet mpool tests
Mise->>ForestDev: run devnet wallet tests
ForestDev-->>CI: pass/fail result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
sudo-shashank
force-pushed
the
shashank/devnet-wallet-test
branch
from
4404020 to
6015fc7
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/state_manager/message_search.rs (1)
103-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
unwrap()from the sequence check.The
parent_actor_state.as_ref().unwrap()on Line 105 is currently safe because the||short-circuits (it's only reached whenparent_actor_stateisSome), but it violates the guideline againstunwrap()and is fragile under future edits (e.g., reordering the||operands would introduce a panic). Restructuring with amatchremoves the unwrap and reads more clearly. The receipt-gating and continue-on-Nonebehavior is preserved.As per coding guidelines: "Avoid
unwrap()in production code; use?orexpect()with descriptive messages instead".♻️ Proposed refactor to remove the guarded unwrap
- if (parent_actor_state.is_none() - || (current_actor_state.sequence > message_sequence - && parent_actor_state.as_ref().unwrap().sequence <= message_sequence)) - && let Some(receipt) = - self.tipset_executed_message(¤t, message, allow_replaced)? - { - return Ok(Some((current, receipt))); - } + let sequence_indicates_execution = match parent_actor_state.as_ref() { + None => true, + Some(parent) => { + current_actor_state.sequence > message_sequence + && parent.sequence <= message_sequence + } + }; + if sequence_indicates_execution + && let Some(receipt) = + self.tipset_executed_message(¤t, message, allow_replaced)? + { + return Ok(Some((current, receipt))); + }🤖 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/state_manager/message_search.rs` around lines 103 - 108, The sequence check in message_search.rs still uses a guarded unwrap on parent_actor_state, which should be removed to match the no-unwrap guideline and avoid future panic risk. Refactor the condition around tipset_executed_message in the relevant branch of the message search flow to use a match on parent_actor_state (or equivalent pattern matching) so the parent sequence comparison only runs when the parent state exists, while preserving the current receipt-gating and continue-on-None behavior.Source: Coding guidelines
src/dev/subcommands/tests_cmd/shared/mpool.rs (1)
39-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace bare
.unwrap()with.expect("...")/.context(...)for CI debuggability.Every external call (
forest_cli,mpool_nonce,poll_until_pending_nonce,poll_until_state_search_msg) uses.unwrap(). On a flaky/slow devnet these will panic with no indication of which address/nonce/command failed, making CI failures harder to triage.♻️ Example fix pattern
- let nonce = mpool_nonce(addr).unwrap(); + let nonce = mpool_nonce(addr).expect("failed to fetch current mpool nonce"); ... - ]) - .unwrap(); + ]) + .expect("mpool nonce-fix (manual gap) failed");As per coding guidelines, "Avoid
unwrap()in production code; use?orexpect()with descriptive messages instead."🤖 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/dev/subcommands/tests_cmd/shared/mpool.rs` around lines 39 - 87, The test helpers in mpool.rs use bare unwraps on external operations, which makes CI failures hard to diagnose. Update mpool_nonce_fix_auto_unblocks_pending and mpool_replace_auto_unblocks_pending to replace unwrap() on forest_cli, mpool_nonce, poll_until_pending_nonce, and poll_until_state_search_msg with expect() or context() carrying the addr, nonce, cid, and command being executed. Keep the assertions, but ensure any failure path reports the specific step and inputs so flaky devnet issues are easier to triage.Source: Coding guidelines
scripts/devnet/wallet_harness.sh (1)
24-36: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrivate key material left unprotected in
/tmp.Both the genesis wallet key (line 25) and the newly funded test wallet key (line 34) are written to
${TMPDIR:-/tmp}in plaintext and never removed or permission-restricted, and themktemp -dkeystore at line 27 is likewise never cleaned up. On a shared or long-lived host this leaves signing key material exposed longer than necessary.🔒 Suggested fix: restrict permissions and clean up on exit
local genesis_key_path="${TMPDIR:-/tmp}/devnet_genesis_wallet.key" docker cp "forest:${LOTUS_DATA_DIR}/genesis-sectors/pre-seal-${MINER_ACTOR_ADDRESS}.key" "${genesis_key_path}" + chmod 600 "${genesis_key_path}" + trap 'rm -f "${genesis_key_path}" "${test_key_path:-}"' RETURN local genesis_addr genesis_addr="$(XDG_DATA_HOME="$(mktemp -d)" ${FOREST_WALLET_PATH} import "${genesis_key_path}")"🤖 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 `@scripts/devnet/wallet_harness.sh` around lines 24 - 36, The wallet setup in wallet_harness.sh leaves sensitive key material in temporary storage without cleanup or permission hardening. Update the genesis_key_path/test_key_path flow in the wallet import/export steps and the temporary XDG_DATA_HOME created for `${FOREST_WALLET_PATH} import` so the files/directories are created with restrictive permissions, and add a cleanup trap to remove them when the script exits. Make sure the changes are applied around the `genesis_addr`, `test_addr`, and `FOREST_TEST_PRELOADED_ADDRESS` setup so both local and remote wallet paths remain functional.mise.toml (1)
228-241: 🧹 Nitpick | 🔵 TrivialLGTM! Confirmed
shell = "bash -c"is valid mise TOML task syntax.Optionally consider tearing down the devnet (
docker compose down) on task exit/failure to avoid leaving containers running across repeated local invocations, but this may be intentional for post-test debugging.🤖 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 `@mise.toml` around lines 228 - 241, The `test:wallet-devnet` task leaves the devnet running after `setup.sh`/`check.sh` and the `forest-dev` test commands complete, which can leak containers across repeated runs. Update the task’s `run` script to add a cleanup step around the existing `scripts/devnet` flow, ideally in a trap or equivalent shell exit handler, so the devnet is torn down even on failure; use the `test:wallet-devnet` task block and the existing `pushd`/`popd` sequence as the place to wire this in.
🤖 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 `@scripts/devnet/wallet_harness.sh`:
- Around line 47-58: The balance check in devnet_wallet_env_init exits early
under set -e if forest-wallet balance fails once, so the retry loop cannot
absorb transient CLI errors. Update the balance probe in wallet_harness.sh to
tolerate nonzero exits from ${FOREST_WALLET_PATH} --remote-wallet balance
"${test_addr}" --exact-balance, then keep retrying until a valid non-"0 FIL"
result is observed or the attempts are exhausted.
- Around line 15-16: The devnet_wallet_env_init helper is changing the caller’s
shell state by enabling -euo pipefail without restoring the previous options.
Save the current shell option state before setting strict mode, and add a RETURN
trap or equivalent cleanup in devnet_wallet_env_init to restore the original
shell options after the function exits so sourcing wallet_harness.sh does not
leave the parent shell modified.
---
Nitpick comments:
In `@mise.toml`:
- Around line 228-241: The `test:wallet-devnet` task leaves the devnet running
after `setup.sh`/`check.sh` and the `forest-dev` test commands complete, which
can leak containers across repeated runs. Update the task’s `run` script to add
a cleanup step around the existing `scripts/devnet` flow, ideally in a trap or
equivalent shell exit handler, so the devnet is torn down even on failure; use
the `test:wallet-devnet` task block and the existing `pushd`/`popd` sequence as
the place to wire this in.
In `@scripts/devnet/wallet_harness.sh`:
- Around line 24-36: The wallet setup in wallet_harness.sh leaves sensitive key
material in temporary storage without cleanup or permission hardening. Update
the genesis_key_path/test_key_path flow in the wallet import/export steps and
the temporary XDG_DATA_HOME created for `${FOREST_WALLET_PATH} import` so the
files/directories are created with restrictive permissions, and add a cleanup
trap to remove them when the script exits. Make sure the changes are applied
around the `genesis_addr`, `test_addr`, and `FOREST_TEST_PRELOADED_ADDRESS`
setup so both local and remote wallet paths remain functional.
In `@src/dev/subcommands/tests_cmd/shared/mpool.rs`:
- Around line 39-87: The test helpers in mpool.rs use bare unwraps on external
operations, which makes CI failures hard to diagnose. Update
mpool_nonce_fix_auto_unblocks_pending and mpool_replace_auto_unblocks_pending to
replace unwrap() on forest_cli, mpool_nonce, poll_until_pending_nonce, and
poll_until_state_search_msg with expect() or context() carrying the addr, nonce,
cid, and command being executed. Keep the assertions, but ensure any failure
path reports the specific step and inputs so flaky devnet issues are easier to
triage.
In `@src/state_manager/message_search.rs`:
- Around line 103-108: The sequence check in message_search.rs still uses a
guarded unwrap on parent_actor_state, which should be removed to match the
no-unwrap guideline and avoid future panic risk. Refactor the condition around
tipset_executed_message in the relevant branch of the message search flow to use
a match on parent_actor_state (or equivalent pattern matching) so the parent
sequence comparison only runs when the parent state exists, while preserving the
current receipt-gating and continue-on-None behavior.
🪄 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: 547c43ce-5281-4c0d-84b7-22970c8f319d
📒 Files selected for processing (11)
.github/workflows/forest.ymlmise.tomlscripts/devnet/README.mdscripts/devnet/wallet_harness.shsrc/dev/subcommands/tests_cmd.rssrc/dev/subcommands/tests_cmd/calibnet/mpool.rssrc/dev/subcommands/tests_cmd/shared.rssrc/dev/subcommands/tests_cmd/shared/helpers.rssrc/dev/subcommands/tests_cmd/shared/mpool.rssrc/dev/subcommands/tests_cmd/shared/wallet.rssrc/state_manager/message_search.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
💤 Files with no reviewable changes (1)
- src/dev/subcommands/tests_cmd/calibnet/mpool.rs
sudo-shashank
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/devnet/wallet_harness.sh (3)
20-27: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrivate key material written to predictable, unprotected /tmp paths with no cleanup.
genesis_key_pathandtest_key_pathare fixed, predictable filenames under${tmp}(shared/tmpby default). The exported/copied key material inherits default umask permissions (nochmod 600), and none of these files — nor themktemp -dthrowawayXDG_DATA_HOMEon line 27 — are ever removed. On a shared or multi-tenant runner this is susceptible to symlink pre-creation races and leaves private keys readable/lingering after the script exits.🔧 Suggested fix
- local genesis_key_path="${tmp}/devnet_genesis_wallet.key" + local genesis_key_path + genesis_key_path="$(mktemp "${tmp}/devnet_genesis_wallet.XXXXXX")" + chmod 600 "${genesis_key_path}" docker cp "forest:${LOTUS_DATA_DIR}/genesis-sectors/pre-seal-${MINER_ACTOR_ADDRESS}.key" "${genesis_key_path}" + trap 'rm -f "${genesis_key_path}" "${test_key_path:-}"' RETURN @@ - test_key_path="${tmp}/devnet_test_wallet.key" - ${FOREST_WALLET_PATH} export "${test_addr}" > "${test_key_path}" + test_key_path="$(mktemp "${tmp}/devnet_test_wallet.XXXXXX")" + chmod 600 "${test_key_path}" + ${FOREST_WALLET_PATH} export "${test_addr}" > "${test_key_path}"Also applies to: 33-37
🤖 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 `@scripts/devnet/wallet_harness.sh` around lines 20 - 27, The wallet harness leaves private key material in predictable /tmp locations and never cleans it up, which risks exposure and symlink races. Update the key handling in wallet_harness.sh around the genesis import flow and the later test key copy/import flow to use unique temporary files/directories created with mktemp, restrict permissions before use, and remove both the copied key files and the throwaway XDG_DATA_HOME directory on exit via a cleanup trap. Use the existing genesis_addr import block and the test key import block as the places to apply the fix.
15-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRemoving
set -euo pipefaildrops fail-fast ondocker exec/docker cpfailures.Compared to the prior revision (flagged in a past review for mutating the caller's shell when sourced), the function no longer sets any strict mode at all. That fixes the sourcing side-effect, but now a failed
docker exec forest cat ...(line 17) ordocker cp(line 25) won't halt the function —token/the genesis key file end up empty/missing and execution continues into later steps with a confusing failure far from the root cause. Since this harness is invoked from CI (mise task / workflow), consider explicit checks on these critical steps instead of relying on implicit propagation.docker exec forest cat "${FOREST_DATA_DIR}/token.jwt" > /dev/null || { echo "ERROR: failed to read token.jwt from forest container" >&2; return 1; }🤖 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 `@scripts/devnet/wallet_harness.sh` around lines 15 - 27, The devnet_wallet_env_init function no longer fails fast on critical docker exec and docker cp operations, so errors can cascade into later steps with misleading failures. Add explicit error handling around the token read and genesis key copy/import path in devnet_wallet_env_init, checking the docker exec forest cat and docker cp commands immediately and returning a clear failure if either one fails. Keep the fix localized to devnet_wallet_env_init so it preserves the no-side-effects behavior while restoring reliable failure propagation.
43-52: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a bounded retry around the final balance read.
--wait-confidence 0only waits for the message to reach chain confidence, and this suite already polls for balance updates elsewhere; a single--exact-balanceread here can still race and fail intermittently.🤖 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 `@scripts/devnet/wallet_harness.sh` around lines 43 - 52, The final balance check in wallet_harness.sh can race because it reads the wallet once immediately after the transfer, so wrap the existing balance lookup in a bounded retry loop before failing. Keep the funding step and the existing balance command using FOREST_WALLET_PATH and --exact-balance, but poll until a non-zero balance is observed or the timeout is reached, then preserve the current error handling if the wallet is still unfunded.
🤖 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 `@scripts/devnet/wallet_harness.sh`:
- Around line 20-27: The wallet harness leaves private key material in
predictable /tmp locations and never cleans it up, which risks exposure and
symlink races. Update the key handling in wallet_harness.sh around the genesis
import flow and the later test key copy/import flow to use unique temporary
files/directories created with mktemp, restrict permissions before use, and
remove both the copied key files and the throwaway XDG_DATA_HOME directory on
exit via a cleanup trap. Use the existing genesis_addr import block and the test
key import block as the places to apply the fix.
- Around line 15-27: The devnet_wallet_env_init function no longer fails fast on
critical docker exec and docker cp operations, so errors can cascade into later
steps with misleading failures. Add explicit error handling around the token
read and genesis key copy/import path in devnet_wallet_env_init, checking the
docker exec forest cat and docker cp commands immediately and returning a clear
failure if either one fails. Keep the fix localized to devnet_wallet_env_init so
it preserves the no-side-effects behavior while restoring reliable failure
propagation.
- Around line 43-52: The final balance check in wallet_harness.sh can race
because it reads the wallet once immediately after the transfer, so wrap the
existing balance lookup in a bounded retry loop before failing. Keep the funding
step and the existing balance command using FOREST_WALLET_PATH and
--exact-balance, but poll until a non-zero balance is observed or the timeout is
reached, then preserve the current error handling if the wallet is still
unfunded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7dc6fb74-829b-45f6-bf4f-19baee986467
📒 Files selected for processing (2)
.github/workflows/forest.ymlscripts/devnet/wallet_harness.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/forest.yml
sudo-shashank
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
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
1 participant
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:
StateManager::search_back_for_message.Walletlabel).mpool nonce-fix --autotest that verifies a nonce gap gets filled.Reference issue to close (if applicable)
Closes #7230
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores