Skip to content

Fix/5205 #5206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 20, 2024
Merged

Fix/5205 #5206

merged 4 commits into from
Sep 20, 2024

Conversation

jcnelson
Copy link
Member

Fixes #5205 so that Nakamoto network state machines run at most once per call to PeerNetwork::run()

@jcnelson jcnelson requested review from kantai and obycode September 18, 2024 01:50
@jcnelson jcnelson requested a review from a team as a code owner September 18, 2024 01:50
obycode
obycode previously approved these changes Sep 18, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but is it feasible to add a test for this behavior?

@jcnelson
Copy link
Member Author

This LGTM, but is it feasible to add a test for this behavior?

Arguably, the entire unit / integration test battery is the test. If this code failed to run the requisite state machines correctly, we'd see widespread failures.

jferrant
jferrant previously approved these changes Sep 18, 2024
@jcnelson
Copy link
Member Author

jcnelson commented Sep 18, 2024

nakamoto_integrations::clarity_burn_state is failing, which is very unexpected

@jcnelson
Copy link
Member Author

nakamoto_integrations::clarity_burn_state passed locally for me. Probably a timing issue that got introduced by speeding up the p2p main loop.

@jferrant jferrant dismissed stale reviews from obycode and themself via 7c073fa September 18, 2024 18:12
…mined in the same stacks block

Signed-off-by: Jacinta Ferrant <[email protected]>
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks for fixing that @jferrant!

@jferrant
Copy link
Contributor

Awesome. Thanks for fixing that @jferrant!

No worries at all :D Thanks for explaining to me!

@wileyj
Copy link
Collaborator

wileyj commented Sep 20, 2024

results of the CI failing test (fyi, the cache has been evicted so a full re-run would be required here).

tests::nakamoto_integrations::multiple_miners)

passes

tests::signer::v0::partial_tenure_fork

RUST_BACKTRACE=full BITCOIND_TEST=1 cargo nextest run \
  --no-capture \
  --verbose \
  --tests \
  --workspace \
  --bin stacks-node \
  --run-ignored all \
  --retries 0  \
  --no-fail-fast \
  -E "test(=tests::signer::v0::partial_tenure_fork)"
...
INFO [1726789407.915984] [stackslib/src/net/p2p.rs:4840] [relayer-http://127.0.0.1:51026] Transaction rejected from mempool, {"error":"transaction rejected","reason":"TooMuchChaining","reason_data":{"actual":34,"expected":27,"is_origin":true,"message":"Nonce would exceed chaining limit in mempool","principal":"ST158X7SZWJVG8KET06C64FSRXD0V1ZT31T72SAR5"},"txid":"2aed82542ae19f3509082950d87990e04dd16838465bc0579da97fd94c968832"}
INFO [1726789407.946710] [stackslib/src/net/rpc.rs:556] [p2p-(127.0.0.1:51023,127.0.0.1:51024)] Handled StacksHTTPRequest, verb: POST, path: /v2/mempool/query?page_id=f2cf05a7cb5725983f36cbfa24dbc82566698aa6dacbdf8b658b706c6c4c28fd, processing_time_ms: 0, latency_ms: 0, conn_id: 685, peer_addr: 127.0.0.1:59792, p2p_msg: None
ERRO [1726789410.844647] [testnet/stacks-node/src/tests/nakamoto_integrations.rs:638] [tests::signer::v0::partial_tenure_fork] Timed out waiting for check to process
ERRO [1726789410.844719] [testnet/stacks-node/src/tests/signer/v0.rs:3819] [tests::signer::v0::partial_tenure_fork] Next tenure failed to tick, fork_initiated?: true, miner_1_tenures: 2, miner_2_tenures: 6, min_miner_1_tenures: 1, min_miner_2_tenures: 1, proposed_before_1: 19, proposed_before_2: 35, mined_before_1: 19, mined_before_2: 0, mined_1: 19, mined_2: 0, proposed_1: 19, proposed_2: 35
thread 'tests::signer::v0::partial_tenure_fork' panicked at testnet/stacks-node/src/tests/signer/v0.rs:3835:25:
explicit panic

tests::signer::v0::retry_on_rejection

ran for over 10 minutes, same scrolling WARN

RUST_BACKTRACE=full BITCOIND_TEST=1 cargo nextest run \
  --no-capture \
  --verbose \
  --tests \
  --workspace \
  --bin stacks-node \
  --run-ignored all \
  --retries 0  \
  --no-fail-fast \
  -E "test(=tests::signer::v0::retry_on_rejection)"
...
WARN [1726789900.947496] [stacks-signer/src/v0/signer.rs:201] [signer_runloop:3002] Cycle #11 Signer #1: Failed to push block to stacks node: Backoff retry timeout occurred. Stacks node may be down.. Retrying...

tests::signer::v0::empty_sortition

RUST_BACKTRACE=full BITCOIND_TEST=1 cargo nextest run \
  --no-capture \
  --verbose \
  --tests \
  --workspace \
  --bin stacks-node \
  --run-ignored all \
  --retries 0  \
  --no-fail-fast \
  -E "test(=tests::signer::v0::empty_sortition)"
  ...
INFO [1726790353.644569] [stackslib/src/net/rpc.rs:556] [p2p-(127.0.0.1:46863,127.0.0.1:65534)] Handled StacksHTTPRequest, verb: GET, path: /v2/info, processing_time_ms: 0, latency_ms: 0, conn_id: 645, peer_addr: 127.0.0.1:38172, p2p_msg: None
ERRO [1726790353.645066] [testnet/stacks-node/src/tests/nakamoto_integrations.rs:623] [tests::signer::v0::empty_sortition] Timed out waiting for block to process, trying to continue test
thread 'tests::signer::v0::empty_sortition' panicked at testnet/stacks-node/src/tests/signer/v0.rs:282:10:
called `Result::unwrap()` on an `Err` value: "Timed out"
stack backtrace:

@obycode
Copy link
Contributor

obycode commented Sep 20, 2024

partial_tenure_fork flakiness is fixed in #5197.

@saralab saralab added this pull request to the merge queue Sep 20, 2024
Merged via the queue into develop with commit a9cb93a Sep 20, 2024
1 check passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 26, 2024
@wileyj wileyj deleted the fix/5205 branch March 11, 2025 21:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants