👍 rkrux approved a pull request: "ci: Make the max number of commits tested explicit"
(https://github.com/bitcoin/bitcoin/pull/33909#pullrequestreview-3517851221)
crACK b5a7a685bba312a780eddcb4a53ce2c26a937854

💬 hodlinator commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2570754863)
"test latest commits" is incrementally better than master. But it is a bit ambiguous - especially for a newcomer who didn't know it by its old name, almost like if it were to be testing on master without any commits from the PR. Curious what others prefer.

👋 fanquake's pull request is ready for review: "[30.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33609)

💬 janb84 commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2570836209)
"test latest commits" would indicate to me that it would test all the commits in the PR (even if >6) , "test latest few commits" would be better imho.

Or even more contrarian, lean into curiosity and make it "test some latest commits"

👍 rkrux approved a pull request: "log: Use more severe log level (warn/err) where appropriate"
(https://github.com/bitcoin/bitcoin/pull/33960#pullrequestreview-3517968266)
crACK fa45a1503eee603059166071857215ea9bd7242a

The removal of the trailing `\n` seems fine as well because it is added later internally if not added by the caller.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2570863123)
Did a tweak commit on top of https://github.com/romanz/bitcoin/commit/9716ff4dd92b31fa4d97b909892f7fc8e3c82b9c: https://github.com/hodlinator/bitcoin/commit/0c75c322a404708d82d9bbf31b70f3b036c831ba

* NotFound -> IO seems more accurate.
* Add comments to explain why we don't log for some errors.
* Marginally nicer errors in rest.cpp (used contrived `0` for unset `part_size`). Feel less strongly about this.

🤔 janb84 reviewed a pull request: "ci: Make the max number of commits tested explicit"
(https://github.com/bitcoin/bitcoin/pull/33909#pullrequestreview-3518075612)
ACK b5a7a685bba312a780eddcb4a53ce2c26a937854

The PR removes incorrectness in the title of the the task "test-each-commit", which doesn't checks each commit, to make it more explicit.

This will help (starting) contributors, in my opinion, to have a better mental model of what is checked in the CI and what isn't. Therefor this is an valuable improvement and worthy of the code churn.

💬 hodlinator commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2570949283)
Maybe do something like this then?

```diff
--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -67,12 +67,15 @@ class RESTTest (BitcoinTestFramework):
status: int = 200,
ret_type: RetType = RetType.JSON,
query_params: Optional[dict[str, typing.Any]] = None,
+ raw_query_params: Optional[str] = None,
) -> typing.Union[http.client.HTTPResponse, bytes, str, None]:
rest_uri = '/rest' + u
...

💬 maflcko commented on pull request "depends: sqlite 3.50.4; switch to autosetup":
(https://github.com/bitcoin/bitcoin/pull/32655#issuecomment-3588516475)
The depends fallback is down again?

```
$ curl --show-headers 'https://bitcoincore.org/depends-sources/sqlite-autoconf-3500400.tar.gz'
HTTP/2 404
server: nginx
date: Fri, 28 Nov 2025 09:21:07 GMT
content-type: text/html
content-length: 146
strict-transport-security: max-age=63072000; includeSubDomains; preload

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2571010317)
> Did a tweak commit on top of [romanz@9716ff4](https://github.com/romanz/bitcoin/commit/9716ff4dd92b31fa4d97b909892f7fc8e3c82b9c): [hodlinator@0c75c32](https://github.com/hodlinator/bitcoin/commit/0c75c322a404708d82d9bbf31b70f3b036c831ba)

lgtm. Could also merge the two size/offset errors into one about just `BadPartRange`?

Seems fine to push as a commit here. It can stay separate, or be squashed, up to you.

💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2571056932)
```suggestion
const auto read_tip_part{[&](auto part_offset, auto part_size){return blockman.ReadRawBlockPart(block_part, tip.GetBlockPos(), part_offset, part_size);}};
BOOST_CHECK(read_tip_part(0, std::nullopt));
```

style nit: Could reduce the verbosity in this line (and below) with a lambda.

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2571094516)
> I must be missing something...

Uff, my sorter ignored the dots and put the smallest numbers first:
```
521546
16519598
18277917
25923656
```

Please resolve :)

✅ maflcko closed an issue: "node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed."
(https://github.com/bitcoin/bitcoin/issues/33948)

✅ fanquake closed a pull request: "ubsan: add another suppression for InsecureRandomContext"
(https://github.com/bitcoin/bitcoin/pull/33949)

💬 fanquake commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3588691024)
I think this is the same issue in our infra.

💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2571219780)
I don't think this one is better than the version on master, newcomers could still be confused by what happens if they only have two commits...

🤔 rkrux reviewed a pull request: "refactor: replace manual promise with SyncWithValidationInterfaceQueue"
(https://github.com/bitcoin/bitcoin/pull/33962#pullrequestreview-3518411840)
ACK d09e9306cab86c0917cc0ae198ef1993d764d106

Looks fine, this function doc highlights the similarity:

https://github.com/bitcoin/bitcoin/blob/808f1d972be35f4c66bdc30ab0f4064dab0c43c0/src/validationinterface.h#L208-L217

💬 rkrux commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2571218739)
Nit: The `callback_set` variable seems unnecessary now.

```diff
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 05450299e5..0b6213f7fd 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -45,7 +45,6 @@ TransactionError BroadcastTransaction(NodeContext& node,

Txid txid = tx->GetHash();
Wtxid wtxid = tx->GetWitnessHash();
- bool callback_set = false;

{
LOCK(cs_main);
@@ -102,22 +101,19 @@ TransactionError BroadcastTransacti
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3588830276)
`e0c9a75fc1...2d398050ee`: add unit test for the private broadcast storage. I could omit it from here and leave it for a followup if reviewers wish?

💬 vasild commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2571374248)
You are right, my bad. Also `FillShortTxIDSelector()` is called from the constructor `CBlockHeaderAndShortTxIDs(const CBlock& block, const uint64_t nonce)`.