💬 maflcko commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2664778729)
Tend toward NACK. This just makes the test harder to edit to adjust the counts for the amounts to be different. Either way, it is harmless and fine to keep as-is.

🤔 zaidmstrr reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2622790831)
Tested ACK [9b293d0](https://github.com/bitcoin/bitcoin/pull/31870/commits/9b293d031c941dc6af0c4274e43388c57c702047)
The idea of fuzzing each algorithm independently seems good. I manually reviewed the new code changes with the previous ones to check for any missing statements or logical errors. I also tested the code by manually inserting assert statements in between to check whether the desired behaviour was occurring or not.

👍 rkrux approved a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2622808152)
I have checked that a coinbase transaction is set to "inactive + abandoned" here on first block disconnect inside the `AddToWallet` function: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1132

And on an unwanted second block disconnection, the same state needs to be passed so that the assertion on line 1114 is satisfied if it's not a new transaction and with the same state variant: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1114

I cherry-p
...

💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959294653)
The same function seems to be used in the first block disconnection as well:
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1129

👍 maflcko approved a pull request: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250#pullrequestreview-2622685585)
Needs rebase.

I left some nits, feel free to ignore.

I skipped over the large commit and the last two.

Otherwise:

review ACK 82fc65c9ab53c416935b5eb20be10fe69ded8b88 👨

<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}"
RUTRmVTMeKV5npGr
...

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959218136)
nit in 321b1cf376865ecc720bbd32357553d508704a35: Should remove the redundant bool now as well?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959226159)
nit in https://github.com/bitcoin/bitcoin/commit/321b1cf376865ecc720bbd32357553d508704a35: Should the test be kept and be done with a descriptor wallet instead?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959293746)
nit in f19592a06059b44d55838bf2f8c63fde2917ee83: Could extend the error msg? Something like:

`"Error: Dumpfile specifies an unsupported database format (%s). Only sqlite database dumps are supported."`?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959251350)
q in bb6a15173eef114c60e620b431a4306f8de213d0: I don't understand this commit. It simply states something and removes code, but I don't see the connection. Maybe a note could help to explain why and when this is needed?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959259156)
nit in 0b84b25e92d382ace40f8b52b80925b3fe76e5f3: Forgot to remove unused `legacy_nodes`?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959279697)
nit in 69eb2c4d85098acbad6b89a09fae4ca84e81fff5: Why set to true redundantly, when no other test does this and when `skip_if_no_wallet` already does it?

💬 maflcko commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1959304120)
eb56556ca4a9104e22cec70698ffceb22719894c: This removes sqlite, but it would be better to remove bdb? Also the commit message needs fixup.

💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959342651)
```diff
- info.prev_hash = tip->phashBlock;
+ info.prev_hash = tip->pprev->phashBlock;
```

The hash of the previous block needs to be added here instead of the hash of the same block. I ran the test with this and it works.

Reference: `MakeBlockInfo()` here https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/kernel/chain.cpp#L18

The `pprev` presence check can be avoided because the test is using `TestChain100Setup` that has 100 blocks mined already lea
...

💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1959355949)
> // to by-pass the wallet birth time.

Can you explain this a bit more? I ran the test w/o it and it works.

💬 dergoegge commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2665081851)
> I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0

It's obviously fine to wait until v30.0 but I don't see why we should? If things on master get additional testing, then we've been testing the headers pre-sync for ~2.5 years now. I don't think this needs anymore testing or what exactly are the expectations here, i.e. what tests are we missing?

👍 TheCharlatan approved a pull request: "cmake: add optional source files to bitcoin_crypto and crc32c directly"
(https://github.com/bitcoin/bitcoin/pull/31268#pullrequestreview-2622958345)
ACK 9cf746d6631739df9c9f80accd5812b319efcfec

👍 TheCharlatan approved a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892#pullrequestreview-2623021656)
ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04

🤔 maflcko reviewed a pull request: "test, refactor: Add TestNode.binaries to hold binary paths"
(https://github.com/bitcoin/bitcoin/pull/31866#pullrequestreview-2622863309)
left a question

💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959395105)
nit in https://github.com/bitcoin/bitcoin/commit/e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: Is there a reason to use types, when a simple dict seems sufficient to store key-value pairs?

💬 maflcko commented on pull request "test, refactor: Add TestNode.binaries to hold binary paths":
(https://github.com/bitcoin/bitcoin/pull/31866#discussion_r1959328055)
nit in e58f5e0e6180e0f0f39d4fb52d1ed822cbde1078: Could name this `cli_argv` to keep inline with the name of the exe and to avoid confusion with the existing `TestNode::rpc` and `TestNode::cli` naming?