💬 ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3469806048)
> The suggested diff doesn't work because we remove the block request from mapBlocksInFlight if the txn's are missing, the firstInFlight slot is already taken, and the block is not from an HB Peer:

Does that logic make sense? If we receive:

```mermaid
sequenceDiagram
participant A
Us ->> A: SENDCMPCT 0
Us ->> B: SENDCMPCT 1 (high-bandwidth)
A ->> Us: t=0 HEADERS
Us ->> A: GETDATA(CMPCT)
B ->> Us: t=25ms CMPCTBLOCK
Us ->> B: GETBLOCKTXN
A ->> Us: t=50ms CMPC
...

💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2479409437)
You are right, although I think it would not be a problem because we don't print that many decimals.

Anyway, I think the approach in afeeac1596da47b0e431d468a665d344b39e1a87 is cleaner and follows what we already do for btc/kvB. What do you think?

💬 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.

💬 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.

💬 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.

💬 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
```

💬 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
...

💬 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.

💬 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.

💬 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
...

💬 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.

💬 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.

💬 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
...

💬 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.

⚠️ roodmandev-ux opened an issue: "1"
(https://github.com/bitcoin/bitcoin/issues/33748)
https://github.com/bitcoin/bitcoin/blob/c05c214f2e9cfd6070a3c7680bfa09358fd9d97a/src/kernel/chainparams.cpp#L116

✅ roodmandev-ux closed an issue: "1"
(https://github.com/bitcoin/bitcoin/issues/33748)

💬 roodmandev-ux commented on issue "1":
(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.

📝 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
...

💬 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
...