Fix/utxo chaining #3110
Fix/utxo chaining #3110
Conversation
…set of epochs, and make it so that we store the sortition burn sample data if the `testing` feature is enabled (so we can query it from integration tests)
…each missed block commit's intended sortition, instead of failing to find the sortition.
…n `stacks`, `clarity`, and `stacks_common`
…y flag for enabling/disabling RBF. If RBF is disabled, then query "unsafe" UTXOs (i.e. those with 0 confirmations) so we can have the test miner send multiple txs per block
…se it, set the `STX_TEST_LATE_BLOCK_COMMIT` environ to the height at which a block-commit should be bumped by one Bitcoin block. When one Bitcoin block has passed, broadcast it late. Note that RBF must be disabled to do this, so we can query 0-conf UTXOs in order to send concurrent block-commits that are not late.
…dule for testing
…and (b) that UTXO chaining is fixed in epoch 2.1 even though there are missed block-commits
…that must be present to build for production
Codecov Report
@@ Coverage Diff @@
## next #3110 +/- ##
==========================================
+ Coverage 81.28% 83.97% +2.68%
==========================================
Files 190 265 +75
Lines 145162 207719 +62557
==========================================
+ Hits 117996 174426 +56430
- Misses 27166 33293 +6127
Continue to review full report at Codecov.
|
testnet/stacks-node/src/neon_node.rs
Outdated
| } else { | ||
| debug!("Mock-mining enabled; not sending Bitcoin transaction"); | ||
| let mut send_tx = true; | ||
| if cfg!(test) { |
There was a problem hiding this comment.
I think this function is already doing a lot for the reader to follow, and they will now have to keep track of a new long cryptic cfg!(test) block.
any way to inject from the outside without putting it into the prod code using some kind of "for test" version of an interface?
an alternative would be to make a function in this file called maybe_test_late_block_commit.
at the end of the day if that's not worth it and you want to check this in it's fine with me.
There was a problem hiding this comment.
I agree with this -- this function is already quite complex, and it seems like this is something that would be pretty easy to at split out to another function:
#[cfg(test)]
fn test_commit_fault_injection(op, op_signer, attempt, bitcoin_controller, block_height) -> bool {
...
}
#[cfg(not(test))]
fn test_commit_fault_injection(op, op_signer, attempt, bitcoin_controller, block_height) -> bool {
true
}
...
let send_commit = test_commit_fault_injection(op, op_signer, attempt, bitcoin_controller, block_height);
if send_commit {
...
}
...| .ok_or_else(|| op_error::BlockCommitNoParent)?; | ||
| let intended_sortition = match epoch.epoch_id { | ||
| StacksEpochId::Epoch21 => { | ||
| // correct behavior |
There was a problem hiding this comment.
maybe "correct behavior, fixes issues/2358"
| if self.block_height < burnchain.first_block_height { | ||
| return Err(op_error::BlockCommitPredatesGenesis); | ||
| } | ||
| let sortition_height = self | ||
| .block_height | ||
| .saturating_sub(burnchain.first_block_height); |
There was a problem hiding this comment.
| if self.block_height < burnchain.first_block_height { | |
| return Err(op_error::BlockCommitPredatesGenesis); | |
| } | |
| let sortition_height = self | |
| .block_height | |
| .saturating_sub(burnchain.first_block_height); | |
| let sortition_height = self | |
| .block_height | |
| .checked_sub(burnchain.first_block_height) | |
| .ok_or_else(|| op_error::BlockCommitPredatesGenesis)?; |
Using checked_sub is more idiomatic than a manual check + saturating sub.
…, and log operation signer disposal
This fixes miner UTXO chaining (#2358), and adds unit tests and integration tests to verify that missed block commits are now included in a miner's UTXO chain when determining its sortition weight.
Nearly all of this new code is tests and test infrastructure. The change itself is less than 20 lines long.