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

💬 maflcko commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3048078577)
I wonder if this can be uncoupled from `CLIENT_VERSION`, because the coupling is confusing and makes testing harder:

* It is confusing, because switching between versions that kept the wallet code the same will pretend that there was a relevant upgrade/downgrade, even though the wallet behavior is exactly identical.
* `CLIENT_VERSION` on the development branch is changed every 6 months, not when a new wallet feature was added that needs to deal with un-upgraded material. I know that running
...

💬 maflcko commented on pull request "doc: fix `BlockConnected` incorrect comment":
(https://github.com/bitcoin/bitcoin/pull/32893#issuecomment-3048091064)
lgtm ACK 4e69aa5701a2dad3805ea26718e6a406adb8b748

🚀 fanquake merged a pull request: "doc: fix `BlockConnected` incorrect comment"
(https://github.com/bitcoin/bitcoin/pull/32893)

💬 maflcko commented on pull request "test: less ambiguous error if bitcoind is missing":
(https://github.com/bitcoin/bitcoin/pull/32921#discussion_r2191997393)
```suggestion
raise AssertionError("At least one binary is missing, did you compile?")
```

there are more binaries that could be missing, than just bitcoind?

💬 maflcko commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#issuecomment-3048207836)
tight polling of is_ibd seems like a mistake in the first place, so i am not sure if this is something to optimize for.

Looking at the remaining call sites of the ibd check, most have cs_main already, so they won't be affected by this? The remaining ones (I only found `MaybeSendFeefilter`), if they are relevant, could either re-order their code to call it less often, or use cache the bool themselves? For the gui, see also https://github.com/bitcoin/bitcoin/issues/17145

💬 maflcko commented on pull request "rest: replace `rf_names[0].rf` by `RESTResponseFormat::UNDEF` for code clarity":
(https://github.com/bitcoin/bitcoin/pull/32884#issuecomment-3048247912)
lgtm ACK 6d19815cd44031ff2b45fc9532f579cd81c62749