💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2214383689)
Or:

> // BLOCK_FAILED_CHILD is deprecated, but may still exist on disk. Replace it with BLOCK_FAILED_VALID.

💬 JeremyRubin commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#issuecomment-3085647936)
i'll check if there are others but this looks cleaner

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214389893)
Thanks for the review.
I'm not a big fan of dead comments, especially when they're duplicating information that the code can tell better.
The name already states that it removes from mempool.
And it's called from ConnectTip in validation.cpp.
I have instead added a comment inside the method without repeating the exact same - please see the commit message.

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214391892)
The code already states clearly that MempoolTransactionsRemovedForBlock is called.

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214400965)
Fair enough, It should be removed then because of the reason here https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3030963395 I've come across multiple places where such comments get stale and not updated.

💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214403826)
This is a valid point, might be better for such removals to be in it's own commit with this explanation as rationale.

💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2214435971)
Since the method is about mempool removal, I found it surprising that it is needed for fee estimation - hence the comment

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3086471740)
re-ACK dbd76e68c3fd86814e96be85a2d23eb542df6e55

Changes from my last ACK cedbc2cd99754c099e92f074e1d5566da265bf26 (git range-diff 7763e86afa...cedbc2cd99 5d17e64a02...dbd76e68c3)

4480ca4a2f - Added a comment
ead32f37c8 - `const uint256&` in some places were changed to `const Txid/Wtxid&` and this caused a conflict
7255b28cbb - fixed issue with passing 0 as second parameter to sendrawtransaction
9ec10ceb59 - split up combined `if` statement into two separate statements. Expanded on a com
...

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2214749431)
You did not change it :(. It is commented out so it's not a deal breaker here.

💬 maflcko commented on pull request "init: [gui] Avoid UB/crash in InitAndLoadChainstate":
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3086779852)
> seems like it got broken multiple times in the past without users really noticing / complaining?

They'd still see the error message about the `-reindex`, before the crash. It seems easy to dismiss a crash as hardware fault and not software fault after you've seen a corruption warning.



> Is there any way to write an actual GUI test for this? Having to add an obscure, single-use option to bitcoind, to simulate crashing behaviour in the GUI, seems like a last resort?

Switched to the
...

💬 maflcko commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3086818557)
See https://github.com/bitcoin/bitcoin/tree/master/contrib/guix#bootstrappable-bitcoin-core-builds

💬 maflcko commented on pull request "test: Do not pass tests on unhandled exceptions":
(https://github.com/bitcoin/bitcoin/pull/33001#discussion_r2214958175)
> I kinda like seeing where it got interrupted

Yeah, I realize it is useful to debug timeouts. Pushed a fresh commit for `KeyboardInterrupt`.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3087403014)
> This increases the scope quite a bit, and if we're going to demonstrate mining, I think it should be done accurately.

I don't think the goal is to _demonstrate_ mining, it's to increase (functional) test coverage.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2215166231)
It seems fine to mine on mainnet for a fraction of a second.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2215171067)
For the purpose of test coverage, it's good to call it this with an invalid solution.

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2215175895)
Regtest has an extremely low difficulty, so it's (almost?) impossible to not find a solution. However it does seem more correct to handle the failure case.

💬 maflcko commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215338569)
This doesn't mean mask is non-zero if non-empty, if that is the goal. Also, if it isn't, IntSan will notify about it:

```
echo 'AQAAAID+/v7+/v7+/n8=' | base64 --decode > /tmp/f.crash

UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=clusterlin_ancestor_finder ./bld/bin/fuzz -runs=1 /tmp/f.crash
```

```
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 1905069321
INFO: Loaded 1 modules (588821
...

💬 maflcko commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3088548960)
review ACK 8810642b571e1d8482375e962a1129b691d5d226 🥁

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8810642b571e
...

🤔 janb84 reviewed a pull request: "cmake: Create subdirectories in build tree in advance"
(https://github.com/bitcoin/bitcoin/pull/32773#pullrequestreview-3032634011)
re ACK fa3beb6316d426b7e388d71fa2f09aed4d2da3d2

Changes sinds last ACK:
- Solved Merge conflict.

Retested, still worked as described. ✅
Code review ✅

💬 mprenditore commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-3088660266)
Hello, is possible to review it again? It should be all good now.
Thanks a lot for the support :)