Skip to content

Conversation

jferrant
Copy link
Contributor

Partially closes #6314

This PR focuses on just removing affirmation code paths from the ChainsCoordinator specifically. A follow on PR will work on removing affirmation.rs entirely and removing DB affirmation map related tables/functions.

@jferrant jferrant requested review from a team as code owners July 24, 2025 16:47
@jferrant jferrant moved this to Status: In Review in Stacks Core Eng Jul 24, 2025
@jferrant jferrant added this to the 3.2.0.0.1 milestone Jul 24, 2025
@jferrant jferrant self-assigned this Jul 24, 2025
@wileyj wileyj moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Jul 25, 2025
@@ -1254,7 +1254,7 @@ fn test_inv_sync_start_reward_cycle() {
let block_scan_start = peer_1
.network
.get_block_scan_start(peer_1.sortdb.as_ref().unwrap());
assert_eq!(block_scan_start, 7);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is correct? But not 100% sure.

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think so. The change to get_block_scan_start appears to have introduced a bug or exposed buggy behavior elsewhere.

Copy link
Contributor Author

@jferrant jferrant Aug 22, 2025

Choose a reason for hiding this comment

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

Hmmm But why should it return 7? The only reason it returned 7 before was when it searched for the heaviest affirmation map it showed the lower 7...now it doesn't exist so it searches for the reward cycle minus inv_reward_cycles which in this case means 9 - 1 which is 8. I am not convinced that the test_peer setup is correct now with affirmation maps being gone, but I logically cannot reason out why this would be anything but 8..

jferrant added 2 commits July 29, 2025 17:03
Signed-off-by: Jacinta Ferrant <[email protected]>
… into chore/remove-affirmation-map-from-chainscoordinator

// affirmation maps are compatible, so just resume scanning off of wherever we are at the
// tip.
// Resume scanning off of wherever we are at the tip.
Copy link
Contributor Author

@jferrant jferrant Aug 7, 2025

Choose a reason for hiding this comment

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

This causes net::tests::inv::nakamoto::test_nakamoto_inv_sync_across_epoch_ to infinitely loop as its calls to inv_getpoxinv_begin always returns reward cycle 7 as the start, but it seems to need reward cycle 2. Not yet figured out why. I assume it is a problem with how the test itself is written but struggling to fix

Copy link
Member

Choose a reason for hiding this comment

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

In that test, can you show me what the output of these info! lines are?

        info!(
            "Epoch 2.x state machine: Peer 1: {inv_1_count}, Peer 2: {inv_2_count} (total {num_epoch2_blocks})"
        );
        info!(
            "Nakamoto state machine: Peer 1: {highest_rc_1}, Peer 2: {highest_rc_2} (total {total_rcs})"
        );

Copy link
Member

Choose a reason for hiding this comment

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

Ah nm I see it in the test logs

Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm that InvSync::sync_inventories_epoch2x() is even being called? The fact that inv_1_count and inv_2_count are both zero means that neither peer has done a round of an epoch2x inventory syncs. It's not indicated by the test logs I'm afraid, since it would require debug-logs to figure out.

Copy link
Contributor Author

@jferrant jferrant Aug 13, 2025

Choose a reason for hiding this comment

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

Yes I confirmed that it is called. See many repeats of the following:

DEBG [1755104776.014222] [stackslib/src/net/inv/epoch2x.rs:2275] [net::tests::inv::nakamoto::test_nakamoto_inv_sync_across_epoch_change] local::60615: Inventory state has 1 block stats tracked on connections {4}
DEBG [1755104776.015142] [stackslib/src/net/inv/epoch2x.rs:2275] [net::tests::inv::nakamoto::test_nakamoto_inv_sync_across_epoch_change] local::60617::pub=127.0.0.1:60617: Inventory state has 1 block stats tracked on connections {3}

@jferrant jferrant requested review from jcnelson and kantai August 12, 2025 18:42
@aldur aldur modified the milestones: 3.2.0.0.1, 3.2.0.0.2 Aug 20, 2025
… into chore/remove-affirmation-map-from-chainscoordinator
…n affirmation map flow around failure to announce blocks

Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant force-pushed the chore/remove-affirmation-map-from-chainscoordinator branch from f69b924 to 3bf47dd Compare August 22, 2025 21:08
jferrant and others added 6 commits August 25, 2025 13:02
…cle as the sync start point, and remove a now-defunct test (PoX bitvectors are always 1's, so there is no need to test for the case when there's a 0)
Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant requested review from hstove and obycode and removed request for hstove August 27, 2025 22:50
… into chore/remove-affirmation-map-from-chainscoordinator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: 💻 In Progress
Development

Successfully merging this pull request may close these issues.

4 participants