Miner changes for time-based tenure extends #5493
Conversation
With this change, the signer will accept a tenure extend from miner N-1 when miner N wins a sortition but commits to the wrong parent tenure.
The previous design using a global singleton causes trouble in testing, when we have multiple miners running in different threads of the same process.
This is useful when checking the behavior during forking.
`SignerDBListener` struct is for a new thread that is always processing StackerDB messages from the signers during a mining tenure. `SignerCoordinator` is the interface that the miner uses with the `SignerDBListener`, to propose a block and wait for signatures.
|
Should the |
Oh, right. It's signer messages in the stacker db. I guess the signerdb is another thing. Oops. Thanks! |
|
This is "ready for review" as it should now 🤞 pass all of the existing integration tests, so it's ready for a first look. The last piece is to enable the actual usage of the tenure extend now that all the pieces are in place. |
|
Forgot to add a comment, but with the last commits, the tenure extend is functional! Still needs more testing and a review of failures in existing tests. |
| /// Tracks signatures for blocks | ||
| /// - key: Sha512Trunc256Sum (signer signature hash) | ||
| /// - value: BlockStatus | ||
| blocks: Arc<(Mutex<HashMap<Sha512Trunc256Sum, BlockStatus>>, Condvar)>, |
There was a problem hiding this comment.
It seems like you have a low-lift opportunity for a future-proofed refactoring here.
In other places, if struct Foo needs to share its state with other threads, it has a corresponding FooComms struct which contains all of these Arc<Mutex<..>>-wrapped data. The FooComms struct then exposes getters and setters for the wrapped data, so the caller doesn't need to bother with knowing how to deal with whatever concurrency primitives are used to access or mutate the Foo instance's data (e.g. locking the mutex or dereferencing the Arc the right way). The Foo implementation has a method or instantiating a FooComms.
It seems like you could have a StackerDBSessionComms struct which contained blocks and signer_idle_timestamps, and gets instantiated from the StackerDBSession.
| ChainstateError::MinerAborted | ||
| })?; | ||
|
|
||
| sc.listener_thread = Some(listener_thread); |
There was a problem hiding this comment.
One of these days, we need to centralize all the singleton thread-creation that happens in the node.
| ) -> Result<Vec<MessageSignature>, NakamotoNodeError> { | ||
| // Add this block to the block status map. | ||
| // Create a scope to drop the lock on the block status map. | ||
| { |
There was a problem hiding this comment.
This scoped locking trick is something we should help callers avoid.
| let mut blocks = lock.lock().expect("FATAL: failed to lock block status"); | ||
|
|
||
| loop { | ||
| let (guard, timeout_result) = cvar |
There was a problem hiding this comment.
This is another example of something we should help callers avoid. Low-level synchronization like waiting on a condition variable can be hidden within a Comms struct.
| } = accepted; | ||
| let tenure_extend_timestamp = response_data.tenure_extend_timestamp; | ||
|
|
||
| let (lock, cvar) = &*self.blocks; |
There was a problem hiding this comment.
This is something a Comms struct could hide from the caller.
| ); | ||
| } | ||
| SignerMessageV0::BlockResponse(BlockResponse::Rejected(rejected_data)) => { | ||
| let (lock, cvar) = &*self.blocks; |
There was a problem hiding this comment.
Same here -- this needlessly marries the synchronization implementation (something that can be hidden) to the business logic.
There was a problem hiding this comment.
My biggest feedback here is that the code mixes a lot of low-level thread synchronization code into the business logic, which I think is something we should strive to avoid. The synchronization logic may change down the road depending on what other threads need to interact with signature-gathering, so we should do what we do elsewhere and wrap all of the thread synchronization / state-sharing behind a "Comms" struct, and provide methods there that better reflect the business logic's needs.
|
Note to reviewers - there is an integration test for this behavior in #5471. |
See #5468
This currently also includes #5452, since it has changes that would conflict. Once that is merged into
develop, I will mergedevelopintofeat/time-based-tenure-extendand then clean this branch up.