stacks-network / stacks-core

The Stacks blockchain implementation
https://docs.stacks.co
GNU General Public License v3.0
3.01k stars 667 forks source link

Fix/5136 node and miner #5138

Closed jcnelson closed 2 weeks ago

jcnelson commented 3 weeks ago

This fixes the node and miner code paths for #5136. It makes it so that the miner will never time out waiting for signatures, and makes it so that the block validation endpoint checks that the proposed block builds on a canonical tenure, and builds on an acceptably-high block in that tenure.

Leaving as a draft for now until integration tests pass, and until additional unit test coverage can be added.

jcnelson commented 3 weeks ago

I'm retargeting this to @jferrant's PR, since her PR subsumes this one anyway. @obycode @jferrant can you take a quick look and approve? This is a subset of @jferrant's PR.

obycode commented 2 weeks ago

I fixed signer_set_rollover in develop but the fix is not present here. That has me worried about other merge issues.

--- a/testnet/stacks-node/src/tests/signer/v0.rs
+++ b/testnet/stacks-node/src/tests/signer/v0.rs
@@ -2832,7 +2832,7 @@ fn signer_set_rollover() {
         .running_nodes
         .btc_regtest_controller
         .get_burnchain()
-        .reward_cycle_to_block_height(next_reward_cycle)
+        .nakamoto_first_block_of_cycle(next_reward_cycle)
         .saturating_add(1);

     info!("---- Mining to next reward set calculation -----");
jcnelson commented 2 weeks ago

@obycode That change is present in this PR

jcnelson commented 2 weeks ago

ping @kantai for a re-approval

jcnelson commented 2 weeks ago

ping @obycode for a re-approval

kantai commented 2 weeks ago

This LGTM, but there are a lot of failing integration tests on this PR. I think those need to be resolved before merging.