💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479440054)
Yes, we do. The reason is explained there. All thread busy, a new task arrives so notifications goes unnoticed, then threads finish processing and sleep. Loosing the new task wake up notification, not running the new task and sleeping until a new one gets submitted. There is a test exercising this exact behavior.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479440054)
Yes, we do. The reason is explained there. All thread busy, a new task arrives so notifications goes unnoticed, then threads finish processing and sleep. Loosing the new task wake up notification, not running the new task and sleeping until a new one gets submitted. There is a test exercising this exact behavior.
💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3470018219)
I was reading the code without having seen the discussions here, only based on PR title and description. With some context from the discussions, I think both of them should be updated to reflect what this PR is really doing. It was asked before (see https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818) but maybe it still needs improvement.
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3470018219)
I was reading the code without having seen the discussions here, only based on PR title and description. With some context from the discussions, I think both of them should be updated to reflect what this PR is really doing. It was asked before (see https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818) but maybe it still needs improvement.
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3470028981)
We actually add peers we send `GETDATA`'s to in response to headers to `mapBlocksInFlight`[^1], so in the scenario you describe we would treat the peer that announced the header to us as `first_in_flight()`.
The flow is [`ProcessHeadersMessage()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b1021dcb3a234f6f219/src/net_processing.cpp#L2829-L2831), and if the header looks good, call [`HeadersDirectFetchBlocks()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b10
...
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3470028981)
We actually add peers we send `GETDATA`'s to in response to headers to `mapBlocksInFlight`[^1], so in the scenario you describe we would treat the peer that announced the header to us as `first_in_flight()`.
The flow is [`ProcessHeadersMessage()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b1021dcb3a234f6f219/src/net_processing.cpp#L2829-L2831), and if the header looks good, call [`HeadersDirectFetchBlocks()`](https://github.com/bitcoin/bitcoin/blob/6f359695c36c13af832b0b10
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479467969)
That is part of the possible future experimentations written in the PR description. Goal is to keep the same synchronization mechanisms we had in the http server code.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479467969)
That is part of the possible future experimentations written in the PR description. Goal is to keep the same synchronization mechanisms we had in the http server code.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479480850)
`num_tasks` is an int. That would require casting it to size_t.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479480850)
`num_tasks` is an int. That would require casting it to size_t.
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2479504434)
Thanks - added in [301116e855...c30647c4d3](https://github.com/bitcoin/bitcoin/compare/301116e85540e49c27473f450fc210b702db4cf5..c30647c4d34c2941696729704854467b30657c43).
Also, tested an invalid RFC 3986 query:
```
$ curl -v 'http://localhost:8332/rest/blockpart/000000000000000000016d31a675b96acb4aacca6e4653039703b9fe03997c78.hex?%XY'
* Host localhost:8332 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
* Trying [::1]:8332...
* Connected to localhost (::1) port 8332
> GET /rest/blockpa
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2479504434)
Thanks - added in [301116e855...c30647c4d3](https://github.com/bitcoin/bitcoin/compare/301116e85540e49c27473f450fc210b702db4cf5..c30647c4d34c2941696729704854467b30657c43).
Also, tested an invalid RFC 3986 query:
```
$ curl -v 'http://localhost:8332/rest/blockpart/000000000000000000016d31a675b96acb4aacca6e4653039703b9fe03997c78.hex?%XY'
* Host localhost:8332 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
* Trying [::1]:8332...
* Connected to localhost (::1) port 8332
> GET /rest/blockpa
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479509680)
This decision was on purpose. To be able to assert and abort the program if a negative number is provided. A negative integer to size_t conversion would be bad.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479509680)
This decision was on purpose. To be able to assert and abort the program if a negative number is provided. A negative integer to size_t conversion would be bad.
✅ roodmandev-ux closed an issue: "1"
(https://github.com/bitcoin/bitcoin/issues/33748)
(https://github.com/bitcoin/bitcoin/issues/33748)
💬 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