📝 davidgumberg opened a pull request: "test: ipc: resolve symlinks in `which capnp`"
(https://github.com/bitcoin/bitcoin/pull/33749)
On Fedora, `/bin/` and `/usr/bin` are symlinked, and on one of my boxes (although I could not reproduce this behavior in a docker container), `/bin` comes before `/usr/bin` in `$PATH`, so `which capnp` reports `/bin/capnp`, and `capnp_dir` is set to `/include`, and the test fails:
```console
$ ./build/test/functional/interface_ipc.py
2025-10-30T20:43:43.753812Z TestFramework (INFO): PRNG seed is: 8370468257027235753
2025-10-30T20:43:43.754163Z TestFramework (INFO): Initializing test direct
...
(https://github.com/bitcoin/bitcoin/pull/33749)
On Fedora, `/bin/` and `/usr/bin` are symlinked, and on one of my boxes (although I could not reproduce this behavior in a docker container), `/bin` comes before `/usr/bin` in `$PATH`, so `which capnp` reports `/bin/capnp`, and `capnp_dir` is set to `/include`, and the test fails:
```console
$ ./build/test/functional/interface_ipc.py
2025-10-30T20:43:43.753812Z TestFramework (INFO): PRNG seed is: 8370468257027235753
2025-10-30T20:43:43.754163Z TestFramework (INFO): Initializing test direct
...
💬 davidgumberg commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479597276)
Strange, this test fails as expected for me with the following diff applied to this branch:
```diff
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index b988e28a3f..485984e437 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -454,9 +454,9 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
block.hashMerkleRoot = BlockMerkleRoot(block);
// Reset cached checks
- block.m_checked_witness_commitment = false;
- block.m_check
...
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479597276)
Strange, this test fails as expected for me with the following diff applied to this branch:
```diff
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index b988e28a3f..485984e437 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -454,9 +454,9 @@ void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t
block.hashMerkleRoot = BlockMerkleRoot(block);
// Reset cached checks
- block.m_checked_witness_commitment = false;
- block.m_check
...
📝 da1sychain opened a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750)
Operating a Bitcoin node across multiple networks poses some fingerprinting risk. [0] Currently, this is not clear from the documentation and may be causing direct harm to users who are unaware of this.
The included documentation change indicates this risk factor but also notes that operating a node across multiple networks does provide an important benefits (increases the cost of eclipse and partitioning attacks) and is thus not discouraged outright.
The i2p documentation did not includ
...
(https://github.com/bitcoin/bitcoin/pull/33750)
Operating a Bitcoin node across multiple networks poses some fingerprinting risk. [0] Currently, this is not clear from the documentation and may be causing direct harm to users who are unaware of this.
The included documentation change indicates this risk factor but also notes that operating a node across multiple networks does provide an important benefits (increases the cost of eclipse and partitioning attacks) and is thus not discouraged outright.
The i2p documentation did not includ
...
💬 TheCharlatan commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479631022)
You're right, I messed something up.
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479631022)
You're right, I messed something up.
💬 TheCharlatan commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479638680)
I previously hinted at resetting this flag too, but does that really make sense, if we've just re-calculated the merkle root?
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479638680)
I previously hinted at resetting this flag too, but does that really make sense, if we've just re-calculated the merkle root?
🤔 mzumsande reviewed a pull request: "refactor: optimize block index comparisons (1.4-6.8x faster)"
(https://github.com/bitcoin/bitcoin/pull/33637#pullrequestreview-3401558839)
Concept ACK
Not 100% sure yet about 2dd0f2ced35a268bcab661c671e7c70271cdd91f, seems like inlining gives most of the speedup, whereas the gain of using the spaceship operator (which comes at the cost of readability) is marginal.
(https://github.com/bitcoin/bitcoin/pull/33637#pullrequestreview-3401558839)
Concept ACK
Not 100% sure yet about 2dd0f2ced35a268bcab661c671e7c70271cdd91f, seems like inlining gives most of the speedup, whereas the gain of using the spaceship operator (which comes at the cost of readability) is marginal.
💬 mzumsande commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479547609)
this looks like a fuzz test, why not make it an actual fuzz target out of this?
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479547609)
this looks like a fuzz test, why not make it an actual fuzz target out of this?
💬 mzumsande commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479542423)
commit msg 08d098947aaeae67e3aeef262df1ecdbdbed744a: wway ->way
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479542423)
commit msg 08d098947aaeae67e3aeef262df1ecdbdbed744a: wway ->way
💬 mzumsande commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479462159)
should take the latest comments from the old function after #29640
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479462159)
should take the latest comments from the old function after #29640
💬 mzumsande commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479610586)
I find "ascending/descending" confusing here, I would have expected the opposites: "descending work" means most work first, but the strict weak ordering or C++ comparators would place the one with the lower work first.
Would suggest something like
"First compare by work (less first), then by earliest activatable time (higher sequence first), then by pointer value (higher first)."
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479610586)
I find "ascending/descending" confusing here, I would have expected the opposites: "descending work" means most work first, but the strict weak ordering or C++ comparators would place the one with the lower work first.
Would suggest something like
"First compare by work (less first), then by earliest activatable time (higher sequence first), then by pointer value (higher first)."
💬 mzumsande commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479555866)
would suggest to add a bit more explanation, e.g. mentioning that old_comparator is a snapshot of an old implementation, so that people who look at this in years understand where it's coming from.
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2479555866)
would suggest to add a bit more explanation, e.g. mentioning that old_comparator is a snapshot of an old implementation, so that people who look at this in years understand where it's coming from.
🤔 jonatack reviewed a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3401868129)
See https://github.com/bitcoin/bitcoin/pull/33498.
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3401868129)
See https://github.com/bitcoin/bitcoin/pull/33498.
👋 optout21's pull request is ready for review: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)
(https://github.com/bitcoin/bitcoin/pull/33661)
👍 ryanofsky approved a pull request: "mining: check witness commitment in submitBlock"
(https://github.com/bitcoin/bitcoin/pull/33745#pullrequestreview-3401822551)
Code review ACK 7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4. Code, test, documentation updates all seem like improvements and it is good to make submitSolution provide better feedback. I do think more improvements could be made here: documentation could be updated to say what requirements on submitSolution inputs are, and I think BlockTemplate accessors could be added so miners do not need to make hardcoded assumptions about the coinbase transaction? (https://github.com/bitcoin/bitcoin/pull/33745#d
...
(https://github.com/bitcoin/bitcoin/pull/33745#pullrequestreview-3401822551)
Code review ACK 7d57c4e44451ce1167ba76a73c9cc6f6fd6cfca4. Code, test, documentation updates all seem like improvements and it is good to make submitSolution provide better feedback. I do think more improvements could be made here: documentation could be updated to say what requirements on submitSolution inputs are, and I think BlockTemplate accessors could be added so miners do not need to make hardcoded assumptions about the coinbase transaction? (https://github.com/bitcoin/bitcoin/pull/33745#d
...
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479687669)
re: https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479638680
> I previously hinted at resetting this flag too, but does that really make sense, if we've just re-calculated the merkle root?
To me, it would seem safest to go other direction and reset all the cached flags including `fChecked`. Or if any flags are intentionally not reset it would be nice to a comment making it clear that it was intentional and should be safe.
In the case of CheckMerkleRoot, it is not just check
...
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479687669)
re: https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479638680
> I previously hinted at resetting this flag too, but does that really make sense, if we've just re-calculated the merkle root?
To me, it would seem safest to go other direction and reset all the cached flags including `fChecked`. Or if any flags are intentionally not reset it would be nice to a comment making it clear that it was intentional and should be safe.
In the case of CheckMerkleRoot, it is not just check
...
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479641553)
In commit "mining: ensure witness commitment check in submitBlock" (504acfcd5d1ed4d77f7fd4f5dc57e66d6718d3d4)
Is there a submitSolution call missing here? I don't understand what this is testing
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479641553)
In commit "mining: ensure witness commitment check in submitBlock" (504acfcd5d1ed4d77f7fd4f5dc57e66d6718d3d4)
Is there a submitSolution call missing here? I don't understand what this is testing
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479711613)
Done in c00612c15592faa094d60c7bba8eb9966f5910d6
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479711613)
Done in c00612c15592faa094d60c7bba8eb9966f5910d6
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479713548)
Agreed -- I was thinking I'd tackle this in #33591, if that seems reasonable?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479713548)
Agreed -- I was thinking I'd tackle this in #33591, if that seems reasonable?
📝 john-moffett opened a pull request: "qt: Remove HD seed reference from blank wallet tooltip"
(https://github.com/bitcoin-core/gui/pull/908)
Blank descriptor wallets currently do not have HD seeds and none can be added (or 'set') by the user, so remove the reference in the tooltip.
As I understand it, descriptor wallets don't have a global HD seed and don't store the HD seeds for keys they generate. Currently, no new HD seeds can be added by the user (even for old wallets since `sethdseed` was removed), though it may be possible in the future, eg - https://github.com/bitcoin/bitcoin/pull/33043
(https://github.com/bitcoin-core/gui/pull/908)
Blank descriptor wallets currently do not have HD seeds and none can be added (or 'set') by the user, so remove the reference in the tooltip.
As I understand it, descriptor wallets don't have a global HD seed and don't store the HD seeds for keys they generate. Currently, no new HD seeds can be added by the user (even for old wallets since `sethdseed` was removed), though it may be possible in the future, eg - https://github.com/bitcoin/bitcoin/pull/33043
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479721696)
I think I managed to squash this into 9d0c8d4d5ec2a50ba4756b9882d8801f24542a2e; hopefully the changes all look sane...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2479721696)
I think I managed to squash this into 9d0c8d4d5ec2a50ba4756b9882d8801f24542a2e; hopefully the changes all look sane...