Support read-count extends in miner and signer #6705
Conversation
* bump the protocol version of signer block responses * add read count tenure extend timestamp to signer block responses * add multiversion protocol deserialize/serialize tests * update multiversion test suit to work with current/new version. the prior tests were broken by the new epoch (the 3.1 signer that they tested against were no longer capable of handling responses about the new epoch) * add integration test for read-count extends * update existing sip-34 e2e tests
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (74.40%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## develop #6705 +/- ##
===========================================
- Coverage 80.03% 74.40% -5.64%
===========================================
Files 577 577
Lines 357651 358108 +457
===========================================
- Hits 286235 266439 -19796
- Misses 71416 91669 +20253
... and 238 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
I feel like I should have comments, but I don't XD LGTM (aside from fmt complaints) |
brice-stacks
left a comment
brice-stacks
left a comment
There was a problem hiding this comment.
Looks good to me. Just a few questions.
| // Do not extend if we have spent a threshold amount of the | ||
| // budget, since it is not necessary. | ||
| let usage = self | ||
| .tenure_budget | ||
| .proportion_largest_dimension(&self.tenure_cost); | ||
| if usage < self.config.miner.tenure_extend_cost_threshold { | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
We talked about removing this since it would prevent a tenure extend from happening when a 51% transaction is the only one in the mempool. Should we make that change here as well?
| // Do not extend if we have spent a threshold amount of the | ||
| // read-count budget, since it is not necessary. | ||
| let usage = self | ||
| .tenure_budget | ||
| .proportion_largest_dimension(&self.tenure_cost); | ||
| if usage < self.config.miner.read_count_extend_cost_threshold { | ||
| info!( | ||
| "Miner: not read-count extending because threshold not reached"; | ||
| "threshold" => self.config.miner.read_count_extend_cost_threshold, | ||
| "usage" => usage | ||
| ); | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
Similar to the above comment, how about we just not add this check?
| self.reason = MinerReason::ReadCountExtend { | ||
| burn_view_consensus_hash: self.burn_block.consensus_hash.clone(), | ||
| }; | ||
| self.mined_blocks = 0; |
There was a problem hiding this comment.
Is this okay to reset for any type of extend? Any side effects from this?
3 participants
This PR implements miner/signer side support for read-count extends (a dimension of SIP-034).