👍 hodlinator approved a pull request: "test: remove scanning check on `wallet_importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/31893#pullrequestreview-2622019300)
cr-ACK 405dd0e647e4ed0402320917a06128eb69504655

### PR description

> May cause intermittent issues. See: #31881

Could be a bit clearer, along the lines of:

Reverts recently merged #31768 due to CI failures, see issue #31881.

💬 brunoerg commented on pull request "test: remove scanning check on `wallet_importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/31893#issuecomment-2664151199)
Thanks, @hodlinator. I updated the description.

🤔 mzumsande reviewed a pull request: "wallet: abandon orphan coinbase txs, and their descendants, during startup"
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2622090730)
Code Review ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2

Another scenario that wasn't explicitly mentioned in the OP is a simple reorg: Wallet has a coinbase tx, then it's unloaded, then the node undergoes a reorg, then the wallet is loaded again, and the coinbase tx should be abandoned. That would also make for a simpler test (no need to copy datadirs).

💬 InnDe commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2664442393)
No luck.
Im think its over for me, I’m not going to work since weeks i think this is
enough for me, thank you all you have been very helpful to be honest please
close the issue

On Tue, 18 Feb 2025 at 00:10 1440000bytes ***@***.***> wrote:

> Now that I look back this feels like social engineering attempt or a troll.
>
> Anyways if @InnDe <https://github.com/InnDe> you are able to access your
> wallet, please let us know.
>
> —
> Reply to this email directly, view it on GitHub
> <h
...

💬 davidgumberg commented on pull request "test: chain reorg for coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/31885#issuecomment-2664444755)
@fjahr It seems to me that there are still parts of `CoinStatsIndex` that could be tested that might be appropriate for a unit test, for example, in `ReverseBlock()` that the hash of the block being reversed [matches the hash of the latest block](https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/index/coinstatsindex.cpp#L426-L429) and some that could be either in a unit test or a functional test e.g. checking that unspendable totals are [reversed](https://githu
...

🤔 BrandonOdiwuor reviewed a pull request: "test: remove scanning check on `wallet_importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/31893#pullrequestreview-2622490135)
Code Review ACK 405dd0e647e4ed0402320917a06128eb69504655

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1959105072)
After thinking about this a bit more and doing some changes to the code, I decided to:

1. Open a separate PR to remove the manual ref counting on `CNode` and replace it with `shared_ptr` and change `CConnman::m_nodes` from `vector<CNode*>` to `unordered_set<shared_ptr<CNode>>`. That PR would be independent from this one.

2. In this PR, change the communication between `SockMan` and `CConman` to be based on pointer to `CNode` instead of node id. Similarly to https://github.com/bitcoin/bitco
...

💬 maflcko commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2664725982)
Absent anyone else being able to reproduce, you could of course also try to reproduce on Linux or macOS. This could rule out an issue specific to your Windows setup.

💬 maflcko commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2664740634)
It is now using FI-light, see https://introspector.oss-fuzz.com/project-profile?project=bitcoin-core and https://github.com/google/oss-fuzz/pull/12983. Though, I am not sure about the takeaways.

💬 maflcko commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2664769789)
> @DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.

It will never be possible to be 100% accurate in a heuristic. If it bugs you, you may be able to work around it by leaving a fresh review comment with a `(concept/approach) ack (commit_id), but I skipped over some commits` (or similar), according to https://github.com/maflcko/DrahtBot/blob/1cce096df7cb4d89d34bcdbed09b20a38fe2a344/webhook_features/src/features/summary_comment.rs#L318-L320. You may also
...

💬 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`?