Chore/remove affirmation map from chainscoordinator #6332
Conversation
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…irmation_maps_at_height, and find_valid_sortitions Signed-off-by: Jacinta Ferrant <[email protected]>
…ain_block Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…k_compatible_with_affirmation_map Signed-off-by: Jacinta Ferrant <[email protected]>
| @@ -2338,45 +1074,18 @@ impl< | |||
| panic!("BUG: no epoch defined at height {}", header.block_height) | |||
| }); | |||
|
|
|||
| if self.config.assume_present_anchor_blocks { | |||
| if cur_epoch.epoch_id >= StacksEpochId::Epoch21 || self.config.assume_present_anchor_blocks | |||
There was a problem hiding this comment.
This if can be removed -- we now always assume that anchor blocks are present, regardless of any hints (i.e. affirmations) from the block-commits. The contained code body must always execute.
There was a problem hiding this comment.
ah I had tried this and it broke a ton of tests and wasn't confident that it was correct. Will do.
There was a problem hiding this comment.
I think that's okay. @kantai I know you wrote quite a few chains coordinator tests which tested its behavior in Stacks 2.x when an anchor block went missing. Can we safely assume that this will never, ever happen now that 2.x has passed, and if so, can we remove your tests?
There was a problem hiding this comment.
In general, I think its better to update tests with the new expected behavior rather than removing them. In this case, tests with missing anchor blocks should stall.
There was a problem hiding this comment.
Okay will try removing the if statement and updating the tests :)
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
… into chore/remove-affirmation-map-from-chainscoordinator
| @@ -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); | |||
There was a problem hiding this comment.
I think this is correct? But not 100% sure.
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…dless of epoch Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
… into chore/remove-affirmation-map-from-chainscoordinator
| // announce a new stacks block to force the chains coordinator | ||
| // to wake up anyways. this isn't free, so we have to make sure | ||
| // the chain-liveness thread doesn't wake up too often | ||
| globals.coord().announce_new_stacks_block(); |
There was a problem hiding this comment.
I feel like I might have a bug here?
There was a problem hiding this comment.
Announcing new burn blocks and stacks blocks is a no-op if there are no blocks present, but we can't just busy-loop doing this because they incur I/O. I think that replacing the "drive chain liveness" code with a loop that just calls announce_new_burn_block() and announce_new_stacks_block() every so often should be enough
| if divergence_rc + 1 >= (heaviest_affirmation_map.len() as u64) { | ||
| // we have unaffirmed PoX anchor blocks that are not yet processed in the sortition history | ||
| debug!("Drive burnchain processing: possible PoX reorg from unprocessed anchor block(s) (sortition tip: {sortition_tip_affirmation_map}, heaviest: {heaviest_affirmation_map}, canonical: {canonical_affirmation_map})"); | ||
| globals.coord().announce_new_burn_block(); |
There was a problem hiding this comment.
Should I still be announcing burn block and stacks block? Feel like something is missing here...
Signed-off-by: Jacinta Ferrant <[email protected]>
…ng out CI Signed-off-by: Jacinta Ferrant <[email protected]>
|
|
||
| // 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. |
There was a problem hiding this comment.
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
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.