Add prom metrics for last mined block #3791
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3791 +/- ##
===========================================
- Coverage 0.18% 0.18% -0.01%
===========================================
Files 301 306 +5
Lines 278555 278640 +85
===========================================
Hits 512 512
- Misses 278043 278128 +85
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
testnet/stacks-node/src/neon_node.rs
Outdated
| @@ -4182,6 +4174,17 @@ impl StacksNode { | |||
| globals.set_initial_leader_key_registration_state(leader_key_registration_state); | |||
|
|
|||
| let relayer_thread = RelayerThread::new(runloop, local_peer.clone(), relayer); | |||
|
|
|||
| let public_key = keychain.get_pub_key(); | |||
| let miner_addr = relayer_thread.bitcoin_controller.get_miner_address(StacksEpochId::Epoch21, &public_key); | |||
There was a problem hiding this comment.
Is this the right epoch to use? Is there a way to use whatever the node's current epoch is?
There was a problem hiding this comment.
I discussed this with @kantai, and he felt that hard-coding this parameter to Epoch21 made sense - want to chime in?
There was a problem hiding this comment.
Yeah, so the issue here is that the "miner address" in the bitcoin controller behaves somewhat surprisingly with epochs and segwit configuration:
if self.config.miner.segwit && epoch_id >= StacksEpochId::Epoch21 {
let hash160 = Hash160::from_data(&public_key.to_bytes_compressed());
BitcoinAddress::from_bytes_segwit_p2wpkh(network_id, &hash160.0)
.expect("Public key incorrect")
} else {
let hash160 = Hash160::from_data(&public_key.to_bytes());
BitcoinAddress::from_bytes_legacy(
network_id,
LegacyBitcoinAddressType::PublicKeyHash,
&hash160.0,
)
.expect("Public key incorrect")
}So, if segwit = true, then the miner mines from two different bitcoin addresses if the epoch boundary is crossed.
As it pertains to the mining monitoring (which is where this variable is used), the monitoring thread assumes a globally static miner address. That's not actually the case, unfortunately, so this is kind of a hack -- if the miner config setting opts into segwit mining, the monitoring code always expects to be watching for a segwit mining address, even if epoch-2.1 hasn't started yet.
I think this is okay in the monitoring code, but I do think it's bad code hygiene, and should have a comment reflecting this logic (and be separated away from the rest of the spawn code into its own method). I think that really the get_miner_address behavior should be changed so that it no longer accepts an epoch parameter, but I understand that probably impacts the integration tests (I cannot imagine a real world miner at any point depended on this code -- it would have required preemptively funding the segwit address for the switch over).
There was a problem hiding this comment.
Looks good to me, but I think you should add a comment to the changes at around lines 4180 in the neon_node and also just move them into a private function like set_monitoring_miner_address() so that its clear that the calculations there are only used by the monitoring code.
kantai
force-pushed
the
prom/mined-block-stat
branch
from
578011d to
36405a5
Compare
|
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. |
Description
Adds prom metrics for last mined block.
This PR also includes a fix to a bug introduced in Epoch 2.1, related to segwit addresses. Essentially, there was a mismatch between the representation of the miner address stored for prometheus and the representation of the miner address within the decoded LeaderBlockCommitOp.
To fix this, it was necessary to update the representation of the miner address within the prometheus related code.
Applicable issues