Feat/tenure change validation #4114
Conversation
…cessed tenures and verifying that tenure-changes account for any missed sortitions or tenures. This deviates from the SIP slightly -- there is at most one TC per block, and we only distinguish between TCs that extend the budget and those that create new tenures.
…ase validation, and tenure table structure and queries
…correctly fill in the tenure-change fields
…not yet linked to a block-commit it has (e.g. if the commit was the first nakamoto commit)
|
Not sure why but CI does not want to run. |
| @@ -156,6 +157,7 @@ impl From<&WSTSSignature> for SchnorrSignature { | |||
| SchnorrSignature(buf) | |||
| } | |||
| } | |||
| */ | |||
|
|
|||
| impl Secp256k1PublicKey { | |||
There was a problem hiding this comment.
Ideally we'd also move away from other libsecp256k1 wrappers. p256k1 wraps everything, including public keys, ecdsa/schnorr sigs, etc.
| parent_microblocks_cost: ExecutionCost::zero(), | ||
| anchored_block_cost: block_execution_cost, | ||
| parent_burn_block_hash, | ||
| parent_burn_block_height, | ||
| parent_burn_block_height: u32::try_from(parent_burn_block_height).unwrap_or(0), // shouldn't be fatal |
There was a problem hiding this comment.
If parent_burn_block_height is to large for u32 should it default to u32::MAX?
There was a problem hiding this comment.
I wanted the error to be very obvious
There was a problem hiding this comment.
This looks good to me, I just had a few comments which are mostly superficial. This PR also currently breaks the stacks-node target, so that must be fixed before it can be approved.
$ cargo check
...
error: could not compile `stacks-node` (bin "stacks-node") due to 7 previous errors; 2 warnings emitted
| @@ -118,45 +118,6 @@ impl Default for Secp256k1PublicKey { | |||
| } | |||
| } | |||
|
|
|||
| pub struct SchnorrSignature(pub [u8; 65]); | |||
There was a problem hiding this comment.
Really nice to see this go XD Didn't like it in there to begin with.
…o return the height, and add a backwards-compatible wrapper. In addition, add a test method for setting the canonical tip
…processed_tenure()
| assert_eq!(matured_reward.parent_miner.coinbase, 1000_000_000); | ||
| } | ||
|
|
||
| if i < 11 { |
There was a problem hiding this comment.
Super nit especially since in a test, but this would be clearer to me with a match on a std::cmp
Like so
match i.cmp(&11) {
Ordering::Less => {...},
Ordering::Greater => {...},
Ordering::Equal => {...},
}
| } else { | ||
| assert_eq!(miner_reward.coinbase, 1000_000_000); | ||
| } | ||
| if i < 10 { |
There was a problem hiding this comment.
Similar to above comment.
…acks-network/stacks-blockchain into feat/tenure-change-validation
|
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. |
This PR addresses #4014 by way of #4053. Tenure-change transactions are now fully validated. In particular:
Leaving as a draft for now so I can add more unit tests and let CI run on the existing ones.