Skip to content

Commit cd0edf2

Browse files
committed
tracing: cast block_connected duration to nanoseconds
When the tracepoint was introduced in 8f37f5c, the connect_block duration was passed in microseconds `µs`. By starting to use steady clock in fabf1cd this changed to nanoseconds `ns`. As the test only checked if the duration value is `> 0` as a plausibility check, this went unnoticed. I detected this when setting up monitoring for block validation time as part of the Great Consensus Cleanup Revival discussion. This change casts the duration explicitly to nanoseconds (as it has been nanoseconds for the last three releases; switching back now would 'break' the broken API again; there don't seem to be many users affected), updates the documentation and adds a check for an upper bound to the tracepoint interface tests. The upper bound is quite lax as mining the block takes much longer than connecting the empty test block. It's however able to detect incorrect duration units passed.
1 parent 9cb9651 commit cd0edf2

File tree

4 files changed

+17
-10
lines changed

4 files changed

+17
-10
lines changed

contrib/tracing/connectblock_benchmark.bt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ usdt:./build/src/bitcoind:validation:block_connected /arg1 >= $1 && (arg1 <= $2
8282
@inputs = @inputs + $inputs;
8383
@sigops = @sigops + $sigops;
8484

85-
@durations = hist($duration / 1000);
85+
@durations = hist($duration / 1e6);
8686

8787
if ($height == $1 && $height != 0) {
8888
@start = nsecs;
@@ -92,7 +92,7 @@ usdt:./build/src/bitcoind:validation:block_connected /arg1 >= $1 && (arg1 <= $2
9292
if ($2 > 0 && $height >= $2) {
9393
@end = nsecs;
9494
$duration = @end - @start;
95-
printf("\nTook %d ms to connect the blocks between height %d and %d.\n", $duration / 1000000, $1, $2);
95+
printf("\nTook %d ms to connect the blocks between height %d and %d.\n", $duration / 1e9, $1, $2);
9696
exit();
9797
}
9898
}
@@ -102,7 +102,7 @@ usdt:./build/src/bitcoind:validation:block_connected /arg1 >= $1 && (arg1 <= $2
102102
blocks where the time it took to connect the block is above the
103103
<logging threshold in ms>.
104104
*/
105-
usdt:./build/src/bitcoind:validation:block_connected / (uint64) arg5 / 1000> $3 /
105+
usdt:./build/src/bitcoind:validation:block_connected / (uint64) arg5 / 1e6 > $3 /
106106
{
107107
$hash = arg0;
108108
$height = (int32) arg1;
@@ -120,7 +120,7 @@ usdt:./build/src/bitcoind:validation:block_connected / (uint64) arg5 / 1000> $3
120120
printf("%02x", $b);
121121
$p -= 1;
122122
}
123-
printf(") %4d tx %5d ins %5d sigops took %4d ms\n", $transactions, $inputs, $sigops, (uint64) $duration / 1000);
123+
printf(") %4d tx %5d ins %5d sigops took %4d ms\n", $transactions, $inputs, $sigops, (uint64) $duration / 1e6);
124124
}
125125

126126

doc/tracing.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ Arguments passed:
106106
3. Transactions in the Block as `uint64`
107107
4. Inputs spend in the Block as `int32`
108108
5. SigOps in the Block (excluding coinbase SigOps) `uint64`
109-
6. Time it took to connect the Block in microseconds (µs) as `uint64`
109+
6. Time it took to connect the Block in nanoseconds (ns) as `uint64`
110110

111111
### Context `utxocache`
112112

src/validation.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2738,7 +2738,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
27382738
block.vtx.size(),
27392739
nInputs,
27402740
nSigOpsCost,
2741-
time_5 - time_start // in microseconds (µs)
2741+
Ticks<std::chrono::nanoseconds>(time_5 - time_start)
27422742
);
27432743

27442744
return true;

test/functional/interface_usdt_validation.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
"""
99

1010
import ctypes
11+
import time
1112

1213
# Test will be skipped if we don't have bcc installed
1314
try:
@@ -105,10 +106,12 @@ def handle_blockconnected(_, data, __):
105106
handle_blockconnected)
106107

107108
self.log.info(f"mine {BLOCKS_EXPECTED} blocks")
108-
block_hashes = self.generatetoaddress(
109-
self.nodes[0], BLOCKS_EXPECTED, ADDRESS_BCRT1_UNSPENDABLE)
110-
for block_hash in block_hashes:
111-
expected_blocks[block_hash] = self.nodes[0].getblock(block_hash, 2)
109+
generatetoaddress_duration = dict()
110+
for _ in range(BLOCKS_EXPECTED):
111+
start = time.time()
112+
hash = self.generatetoaddress(self.nodes[0], 1, ADDRESS_BCRT1_UNSPENDABLE)[0]
113+
generatetoaddress_duration[hash] = (time.time() - start) * 1e9 # in nanoseconds
114+
expected_blocks[hash] = self.nodes[0].getblock(hash, 2)
112115

113116
bpf.perf_buffer_poll(timeout=200)
114117

@@ -123,6 +126,10 @@ def handle_blockconnected(_, data, __):
123126
assert_equal(0, event.sigops) # no sigops in coinbase tx
124127
# only plausibility checks
125128
assert event.duration > 0
129+
# generatetoaddress (mining and connecting) takes longer than
130+
# connecting the block. In case the duration unit is off, we'll
131+
# detect it with this assert.
132+
assert event.duration < generatetoaddress_duration[block_hash]
126133
del expected_blocks[block_hash]
127134
assert_equal(BLOCKS_EXPECTED, len(events))
128135
assert_equal(0, len(expected_blocks))

0 commit comments

Comments
 (0)