Skip to content

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Mar 9, 2024

In looking at testnet logs, I've seen that we hit scenarios where a block is saved as "accepted", which deletes the block from SignerDB, and then shortly later the signer tries to process the block, but the signer has no about the block (because it's deleted).

image

@hstove hstove requested review from kantai and obycode March 9, 2024 17:53
Copy link

codecov bot commented Mar 9, 2024

Codecov Report

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

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

Additional details and impacted files
@@                 Coverage Diff                  @@
##           dream-team-fixes    #4514      +/-   ##
====================================================
- Coverage             83.23%   82.83%   -0.40%     
====================================================
  Files                   452      452              
  Lines                326113   326161      +48     
  Branches                323      323              
====================================================
- Hits                 271436   270177    -1259     
- Misses                54669    55976    +1307     
  Partials                  8        8              
Files Coverage Δ
stacks-signer/src/signer.rs 70.17% <ø> (-7.03%) ⬇️
stacks-signer/src/signerdb.rs 80.00% <50.00%> (-8.13%) ⬇️

... and 33 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...d1d1365. Read the comment docs.

Comment on lines -995 to +1000
// TODO: proper garbage collection...This is currently our only cleanup of blocks
self.signer_db
.remove_block(&block_vote.signer_signature_hash)
.expect(&format!("{self}: Failed to remove block from to signer DB"));
// WIP: try not deleting a block from signerDB until we have a better garbage collection strategy.
// This causes issues when we have to reprocess a block and we have already deleted it from the signerDB
// // TODO: proper garbage collection...This is currently our only cleanup of blocks
// self.signer_db
// .remove_block(&block_vote.signer_signature_hash)
// .expect(&format!("{self}: Failed to remove block from to signer DB"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep -- I'd advocate just dropping this line permanently (no need to comment out). Now that this information is stored in a file, I don't think that there's a strong need to clean this state up.

@obycode obycode merged commit d79d26b into dream-team-fixes Mar 9, 2024
@obycode obycode deleted the fix/dont-remove-blocks branch March 9, 2024 20:52
@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