Repository navigation
fix: trust_peer_addr flaky test; atomic fetch_update -> try_update - #1205
Conversation
Rust 1.99 deprecates the atomic fetch_update as a rename of try_update. try_update is stable since 1.95.0, our MSRV, and fetch_update only forwards to it, so behavior does not change.
trust_peer_addr asserts the parsed default max_inbound is DEFAULT_MAX_INBOUND (125), but parsing absorbs RBITCOIN_P2P_MAX_INBOUND. cli_omit_preserves_advanced_env sets that env to 91 under OPERATOR_ENV_TEST_LOCK for a whole --smoke run, and trust_peer_addr did not take the lock. When the two overlap, `plain` reads 91. On master with rustc 1.95, the node suite failed 8 of 10 runs and the two tests alone failed 20 of 20. With the lock, 10 of 10 and 20 of 20 pass.
phlip9
changed the title
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
APPROVE. 0 bugs, 0 suggestions, 0 nits.
No review comments are on the pull request, including from coreyphillips.
Atomic*::try_update is the same operation as fetch_update: it retries the compare-and-swap, returns Ok(previous), and takes the same success and failure orderings. It has been stable since Rust 1.95.0, which is this tree's minimum compiler, and fetch_update only forwards to it from the 1.99 deprecation. Every fetch_update in the tree is renamed, and the orderings at those sites stay legal (Relaxed, Acquire, Release, AcqRel, SeqCst). Callers that use the Ok value (note_batch_depth_send, note_send_written, note_spend_annotate_pending) still see the previous word.
trust_peer_addr now holds OPERATOR_ENV_TEST_LOCK for the whole parse, which is the mutex cli_omit_preserves_advanced_env already holds while RBITCOIN_P2P_MAX_INBOUND=91 is set. That assertion is the only check of the default inbound cap in the cli test binary. --trust-peer still forces max_inbound to 0 after the env is absorbed, so the sibling tests in that binary do not depend on the default. Nothing operator-visible changes, so there is no changelog.d note.
Two small fixes:
atomics:
fetch_update→try_update:AtomicU64::fetch_update(and others) and so we get warnings when building with a newer compilertry_updateis stable since 1.95.0, which is also our MSRV.fetch_updateis now just an alias fortry_updatefix flaky
cli::trust_peer_addrtest, which races onRBITCOIN_P2P_MAX_INBOUNDtrust_peer_addrassertsmax_inbound == DEFAULT_MAX_INBOUND, butoperator_config_from_argsabsorbsRBITCOIN_P2P_MAX_INBOUND.cli_omit_preserves_advanced_envsets that env to91underOPERATOR_ENV_TEST_LOCKfor a whole--smokerun, andtrust_peer_addrdid not take the lock:trust_peer_addrnow takes the lock, like the other env-reading tests.There's a more systematic fix for this issue (don't set/remove envs in tests), but I'll post that in a followup PR. These are just the basic fixes.