✅ l0rinc closed a pull request: "kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate`"
(https://github.com/bitcoin/bitcoin/pull/31362)

💬 l0rinc commented on pull request "kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate`":
(https://github.com/bitcoin/bitcoin/pull/31362#issuecomment-2503982559)
Closing in favor of https://github.com/bitcoin/bitcoin/pull/31382

💬 fanquake commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1860735726)
I don't really have one, given it's a somewhat obscure issue, but which is also why it'd be good to have proper documentation for. Anyone reading this in future isn't going to have any information other than, "don't use ffile-prefix-map otherwise it might break something in oss-fuzz". However given how long the build has been broken now, going to merge this to unbreak it.

💬 l0rinc commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1860729040)
nit: this being a console app, it might make sense to return a non-zero error code when `cerr` is used

🤔 l0rinc reviewed a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382#pullrequestreview-2465133813)
Concept ACK

Closed https://github.com/bitcoin/bitcoin/pull/31362 in favor of this

🚀 fanquake merged a pull request: "build: Fix coverage builds"
(https://github.com/bitcoin/bitcoin/pull/31337)

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1860756141)
Yeah, done.

💬 maflcko commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-2504013215)
> It may be better to modify `CAutoFile` in that case. The file handle in the auto-file should[1] always be unique and pinned to a single thread, so I using `fread_unlocked` seems better, as it improves performance everywhere. (Though, it looks like `std::fread_unlocked` does not exist in C++, so I wonder if it exists on Windows and macOS?)
>
> [1] I may submit a related pull to better enforce this at compile-time.

For reference, `AutoFile::Get` was removed in commit a240e150e837b5a95ed197
...

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2504021426)
reACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21


blockhash is now an optional return

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2504098705)
Seems like I am running into this here: https://github.com/bitcoin/bitcoin/pull/25073#issuecomment-1194417234, maybe time to introduce a replace mempool method like Carl suggested there?

✅ yancyribbens closed a pull request: "Add coin-grinder example test"
(https://github.com/bitcoin/bitcoin/pull/31352)

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860890755)
> The problem is that no one will notice if this isn't run on any machine

I changed it to insist that the tcpdump file was created on the ASAN env. So the ASAN job will be red if this stops working on it.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1860910578)
I'd like to revive my original concerns here: [`c4eb31b` (#30906)](https://github.com/bitcoin/bitcoin/pull/30906/commits/c4eb31be3e56936479921b7556fcba1747867d02#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33L778-L782) removes this method completely, which I think is a better fix for it than the current workaround.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1860914367)
I'm not sure I understand the reason for the suppression, please see my original suggestion in https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700

💬 furszy commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2504309436)
> Ruling out consensus-invalid scripts makes sense, but discarding non-standard scripts is a much bigger leap. Also it would be a wack-a-mole: does the migration crash on any non-standard script? How much of that do you want to cover?

Ok, I might have made the PR/commit description too broad when the actual changes are very specific.
Migration does not crash on all non-standard scripts, it only crashes on scripts that fail to be parsed by the descriptors' logic. The specific non-standard cra
...

🤔 mzumsande reviewed a pull request: "rpc: Remove submitblock pre-checks"
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2465507312)
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
Reviewed the code (didn't look closely at `bitcoin-chainstate.cpp`) and tested it a little bit.

💬 mzumsande commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1860960957)
nit: If, after this PR, we re-submit genesis via `submitblock`, a different code path will be taken than for non-genesis duplicate blocks:
We won't return early in `AcceptBlockHeader`, will actually call `AddToBlockIndex()`, and write an incorrect log message
`Saw new header hash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 height=0` for each `submitblock` call.
Not a real problem though because nothing fails and why would you do that in the first place.

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2504369867)
Updated 1f18eefe1e5bc3a97f5f6c9637a3a193e2c2f22e -> 27072547bb22cdd2080d1014eba9c30bc9d47650 ([`pr/mine.13`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.13) -> [`pr/mine.14`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.14), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.13..pr/mine.14)) to fix CI errors from `skip_if_no_multiprocess` check being broken and from default testdir path exceeding maximum socket length

💬 achow101 commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2504381025)
ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2504393638)
re-ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21