🤔 stickies-v reviewed a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3403430467)
Concept ACK: reindex validation behaviour being different to IBD validation behaviour is undesirable and should be avoided.

Slight approach NACK: I think this is probably the smallest diff we can get to improve reindex behaviour, but I think there are drawbacks that outweigh the benefits:
1) it still doesn't make IBD and reindex validation behaviour the same: with the current approach blocks that are NOT part of the assumevalid chain will still be validated as assumevalid. While it doesn't s
...

✅ fanquake closed an issue: "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null"
(https://github.com/bitcoin/bitcoin/issues/33643)

🚀 fanquake merged a pull request: "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn"
(https://github.com/bitcoin/bitcoin/pull/33743)

💬 fanquake commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3472257325)
ACK facf8b771a192ba17c8c4f9c43f248d83b3c8015

🚀 fanquake merged a pull request: "ci: Add missing python3-dev package for riscv64"
(https://github.com/bitcoin/bitcoin/pull/33746)

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2481027571)
I wanted to use the SQL [order-by](https://www.postgresql.org/docs/current/queries-order.html#QUERIES-ORDER) terminology here
> Ascending order puts smaller values first, where “smaller” is defined in terms of the < operator.
Similarly, descending order is determined with the > operator.

But you're right, I inverted the terminology, added a dedicated test to check for hard-coded order (first param is increasing, second is decreasing), updated the comments (this is why I find comments a lazy
...

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2481027700)
I strongly dislike code comments, I prefer explaining with live code over dead comments - and if people disagree they can always do a blame which instantly reveals the purpose since I also over-explain in commit messages usually.
Are the names `old_comparator` in a `cblockindex_comparator_equivalence ` not enough to make it obvious that? I
I have added a comment anyway, let me know if it helps.

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2481027916)
I look at fuzz tests as exploratory tools that are hard to write and run and debug and do code coverage on - not trivial to maintain (see https://github.com/bitcoin/bitcoin/issues/33731), it's definitely not my go-to way to test something. This is just a randomized property based unit test - easy to debug, easy to run and understand.
But looking at the tests again, I can make it more deterministic so that it hits all important branches which allows me to reduce the iteration count - code covera
...

💬 l0rinc commented on pull request "refactor: optimize block index comparisons (1.4-6.8x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2481029377)
done

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3472525510)
I split the template block mutation test into a third commit and documented its history in the commit message.

📝 rustaceanrob opened a pull request: "test: Format strings in `test_runner`"
(https://github.com/bitcoin/bitcoin/pull/33753)
`format!` strings may contain variables within the string representation. This is a lint as of a recent `rustc` nightly version.

Pull requests without a rationale and clear improvement may be closed
immediately.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3472571178)
> 1. Log an error that assumevalid is enabled but reindex could not find the specified assumevalid header, print the highest available header in blockindex and inform the user that they can restart with `-assumevalid=0` or `-assumevalid=<best_available_header>`or a fresh IBD, and then shutdown the node.

I agree with this approach, but we would still have to update the `minimumchainwork` logic for reindex, or we ask the user to also update the `minimumchainwork` too.

> 3. generalize validat
...

📝 maflcko opened a pull request: " ci: gha: Set debug_pull_request_number_str annotation "
(https://github.com/bitcoin/bitcoin/pull/33754)
GitHub Actions does not offer any way to determine the pull request number in a machine readable way from the checks API. See https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1503475232.

However, the pull request number can be useful for external tools to act on CI results.

Fix that by using a check run annotation for a single task named `debug_pull_request_number_str`.

This should re-enable the 'CI Failed' labelling mechanism via https://github.com/maflcko/DrahtBot/commit/1
...

💬 maflcko commented on pull request "test: Format strings in `test_runner`":
(https://github.com/bitcoin/bitcoin/pull/33753#issuecomment-3472587205)
Thanks. It makes sense to be clippy-clean and just consistently apply the shorter and easier-to-read variant.

lgtm ACK d305b865c9630646a6cf7d227c08ba4263a379f7

💬 maflcko commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481100715)
this seems a bit tedious. Why not just use i64 as a type, which can hold a sign and a value. This way, it won't be necessary to use a bool to hold the sign.

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3472631221)
Concept ACK

Can there ever only be one `waitNext()` call on per `BlockTemplate`? If not, does `interruptWait()` just stop all of them?

💬 Sjors commented on pull request "test: ipc: resolve symlinks in `which capnp`":
(https://github.com/bitcoin/bitcoin/pull/33749#issuecomment-3472661703)
I tested that it doesn't interfere with my macOS setup (`/opt/homebrew/bin/capnp`).

💬 maflcko commented on pull request "ci: gha: Set debug_pull_request_number_str annotation":
(https://github.com/bitcoin/bitcoin/pull/33754#issuecomment-3472734162)
(looks like it worked)

💬 TheCharlatan commented on pull request "init: Fix Ctrl-C shutdown hangs during wait calls":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3472793534)
rfm?

💬 Sjors commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3472829721)
> Plan

Is this the plan for the current PR? Otherwise maybe it makes more sense to move that to the tracking issue.