💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2479414797)
I think with the rebase it's easier to change defaults without touching code logic.
What would be the benefits of using `self.MaybeArg<bool>`? I'm not really familiar with it tbh.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2479414797)
I think with the rebase it's easier to change defaults without touching code logic.
What would be the benefits of using `self.MaybeArg<bool>`? I'm not really familiar with it tbh.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479418360)
It is a known issue. The pcp fuzz test does it too for the same reason (in an undocumented manner). I only document it properly so we don't forget this exists. Feel free to investigate it in a separate issue/PR. I'm not planning to do it. It is not an issue of the code introduced in this PR.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2479418360)
It is a known issue. The pcp fuzz test does it too for the same reason (in an undocumented manner). I only document it properly so we don't forget this exists. Feel free to investigate it in a separate issue/PR. I'm not planning to do it. It is not an issue of the code introduced in this PR.
💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479421965)
aeb18a134620c61c4518a357fea651e7c8d1a024: This could be simplified. These strings ("Send ... BTC") are not being used.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479421965)
aeb18a134620c61c4518a357fea651e7c8d1a024: This could be simplified. These strings ("Send ... BTC") are not being used.
💬 brunoerg commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479434096)
aeb18a134620c61c4518a357fea651e7c8d1a024: This `assert_equal` seems unnecessary. You're basically doing:
```py
a = b
assert a == b
```
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2479434096)
aeb18a134620c61c4518a357fea651e7c8d1a024: This `assert_equal` seems unnecessary. You're basically doing:
```py
a = b
assert a == b
```
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3469979496)
@maflcko thanks for reviewing :)
> I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
I don't think there is either, at least not in a backwards compatible way.
> There could be a global option to toggle the default, to improve the UX.
That would be great for a `bitcoin-cli` user, but probably fatal for other programs that rely on the Bitcoin Core RPC.
E.g. if a user with a Lightning Node enables sat/vB, thu
...
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3469979496)
@maflcko thanks for reviewing :)
> I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.
I don't think there is either, at least not in a backwards compatible way.
> There could be a global option to toggle the default, to improve the UX.
That would be great for a `bitcoin-cli` user, but probably fatal for other programs that rely on the Bitcoin Core RPC.
E.g. if a user with a Lightning Node enables sat/vB, thu
...
💬 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.