Skip to content

Conversation

jferrant
Copy link
Contributor

@jferrant jferrant commented Mar 9, 2024

Signers need to remove outdated reward cycle signer states from their map

@jferrant jferrant requested review from hstove, kantai and obycode March 9, 2024 14:24
@kantai kantai changed the base branch from next to dream-team-fixes March 9, 2024 14:26
Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 83.25%. Comparing base (d56895a) to head (59f2377).
Report is 7 commits behind head on dream-team-fixes.

Additional details and impacted files
@@                 Coverage Diff                  @@
##           dream-team-fixes    #4513      +/-   ##
====================================================
+ Coverage             83.23%   83.25%   +0.02%     
====================================================
  Files                   452      452              
  Lines                326113   326170      +57     
  Branches                323      323              
====================================================
+ Hits                 271436   271560     +124     
+ Misses                54669    54602      -67     
  Partials                  8        8              
Files Coverage Δ
testnet/stacks-node/src/nakamoto_node/miner.rs 80.13% <ø> (+0.05%) ⬆️
stacks-signer/src/signer.rs 76.18% <0.00%> (-1.02%) ⬇️
stacks-signer/src/runloop.rs 90.65% <78.57%> (-0.57%) ⬇️

... and 20 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a736360...59f2377. Read the comment docs.

@obycode obycode force-pushed the bugfix/cleanup-outdated-signers branch from 6dd744d to 59f2377 Compare March 9, 2024 16:18
debug!("{signer}: Signer's tenure has completed.");
// We don't really need this state, but it's useful for debugging
signer.state = SignerState::TenureCompleted;
to_delete.push(signer.reward_cycle % 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still going through this logic to understand what's happening, but my current question is how do we ensure that the signer for say cycle N-2 does not cause the signer for cycle N to get deleted?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it seems a little strange that this HashMap is indexed by whether the reward cycle is even or odd. I get that we only care about either last/current, or current/next cycles. But it seems like there should be a cleaner way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again, I see why you're doing this. Really I'd like to just do the following before the loop, but then we don't get a log for each one.

        // Filter out signers for cycles which have now passed.
        self.stacks_signers
            .retain(|_, signer| signer.reward_cycle >= current_reward_cycle);

Copy link
Contributor

Choose a reason for hiding this comment

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

I just pushed a commit to make it a little more clear.

@obycode obycode merged commit 249ee53 into dream-team-fixes Mar 9, 2024
@obycode obycode deleted the bugfix/cleanup-outdated-signers branch March 9, 2024 21:55
@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 Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants