💬 1440000bytes commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3047520707)
> The test should probably be added to the PR after the commit fixing it

I agree

💬 1440000bytes commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3047522544)
> I don't think the test is useful.
>
> Other than that, concept ACK.

You can ignore it.

💬 stratospher commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#issuecomment-3047561903)
> In InvalidateBlock, both the block failure flags and m_best_header are kept up to date with each block that is disconnected, so it's not necessary to recalculate them at the end of this function.

maybe I'm missing something but even without the `calc_flags_and_header=false` flag, this means that [the condition](https://github.com/bitcoin/bitcoin/blob/a8bff38236ac988f82dcfa85438946cfe0d3afe3/src/validation.cpp#L2074) cannot hit true and `RecalculateBestHeader()` in `InvalidChainFound()` cann
...

💬 Sjors commented on pull request "rpc: use anti-fee-sniping in send, sendall and walletcreatefundedpsbt":
(https://github.com/bitcoin/bitcoin/pull/32892#issuecomment-3047642328)
@glozow thanks, I hadn't seen that one. Will take a look.

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3047748032)
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f

@achow101 can you make a detached signature for either macOS or Windows for the current commit? It seems like a good precaution to test that given: https://github.com/bitcoin/bitcoin/commit/03f585d58ccf4d5c02d621c5b6046d45807b3201

(I'll link to my non code-signed guix build when it finishes)

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2191742180)
Looks great with `mdcat` :-)

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3047771879)
Linter is unhappy over an `unused import` in the test.

👍 stratospher approved a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2996421512)
reACK 8cc3ac6.

💬 maflcko commented on pull request "ci: Catch tests corrupting the source directory":
(https://github.com/bitcoin/bitcoin/pull/32874#issuecomment-3047791077)
> Better not to trust CI to do a security audit for you...

Yeah, anything in the CI is more a belt-and-suspenders and a way to catch "sunny-day" violations.

Generally, the approach of removing write access can only catch unintentional and accidental violations anyway, because an intentional violation could just guard itself when a write error happens due to missing access. So any violation would have to directly lead to the process being killed, or otherwise logged to mark the tests as fai
...

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191837500)
why is this needed? A pointer can already hold the state of null, so i don't see why the translation to optional is needed

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191840823)
this just makes it confusing to special-case string here and doesn't seem ideal having to create a full copy of the possibly long string for no reason

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2191846030)
why is this a member method? It doesn't use any member fields. Also, it seems a bit too trivial to create a helper for a method that is basically a one-line wrapper around `std::ranges::all_of`. Seems easier to just inline it in the rare cases where it is needed.

📝 Sjors opened a pull request: "test: less ambiguous error if bitcoind is missing"
(https://github.com/bitcoin/bitcoin/pull/32921)
Before this change, when a functional test is run without building the source, the error message suggested that previous release binaries were missing.

When no previous release version is set, make the error message more specifically about bitcoind.

💬 0xB10C commented on pull request "POC: IPC tracing interface":
(https://github.com/bitcoin/bitcoin/pull/32898#issuecomment-3047927576)
Awesome, thanks! I'll play around with this over the next few weeks.

💬 maflcko commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2191871702)

“BnB does not add permit adding more inputs to a solution…” -> “BnB does not permit adding more inputs to a solution…” [remove the extraneous “add” to correct the verb phrase]

💬 fanquake commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-3047959957)
@vasild Are you planning on responding to the (remaining) outstanding comments here? Or opening a followup?

💬 maflcko commented on pull request "ci: Use optimized Debug build type in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3047960654)
To provide some numbers that the new checks really are roughly for free:

This ci run took `30min7sec` (https://github.com/bitcoin/bitcoin/actions/runs/16112781421/job/45459901801?pr=32888)

Previously (randomly last month) it took `30min14sec` (https://github.com/bitcoin/bitcoin/pull/32680/checks)

💬 0xB10C commented on issue "Tracepoint Interface Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/31274#issuecomment-3047972436)
There has been some discussion about having an alternative IPC-based tracing interface in https://github.com/bitcoin-core/libmultiprocess/issues/185 and https://github.com/bitcoin/bitcoin/pull/32898. We don't yet know if it causes too much overhead, but it would resolve the following pain-points if the the current eBPF based tracing interface is replaced with an IPC one at some point:

- IPC is available on all platforms, which means it would include macOS, FreeBSD, and even Windows out of the
...

💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3048010967)
ref https://github.com/bitcoin/bitcoin/pull/32896

💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3048021752)
> PTAL

Not sure what you are asking for. Feedback has been provided, and the burden to address it and reply to it is on the pull request author. I can't make it go away by looking at it more.

Apart from the prior feedback, you'd also have to squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits into atomic units (https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches)