💬 roodmandev-ux commented on issue "1":
(https://github.com/bitcoin/bitcoin/issues/33748#issuecomment-3470300182)
-
(https://github.com/bitcoin/bitcoin/issues/33748#issuecomment-3470300182)
-
💬 TheCharlatan commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479587224)
In commit 504acfcd5d1ed4d77f7fd4f5dc57e66d6718d3d4:
If I apply this test independently before the miner.cpp code changes, it still passes, so not sure what we are testing here. Wouldn't this only be relevant if we'd run checkBlock on the template's own block? Afaict we only do checkBlock on blocks we re-create here.
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2479587224)
In commit 504acfcd5d1ed4d77f7fd4f5dc57e66d6718d3d4:
If I apply this test independently before the miner.cpp code changes, it still passes, so not sure what we are testing here. Wouldn't this only be relevant if we'd run checkBlock on the template's own block? Afaict we only do checkBlock on blocks we re-create here.
📝 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?