💬 hodlinator commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2664092403)
> But... this test is not deterministic. If the scanning process runs slower, the "0" duration check will also fail. I'm not sure we should keep it.

Yeah, it seems like an overly harsh check to me. Curious what was the impulse behind #31768 @brunoerg?

💬 fjahr commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2664099485)
re-ACK debacd1f59641c1e9e268c20820f4c8f0fc583f7

Confirmed all the BIP373 test vectors are present in the PR.

💬 brunoerg commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2664104230)
> I haven't tried, but https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1954481759 by @hodlinator may fix it

Yes, it fixes it.

> Yeah, it seems like an overly harsh check to me. Curious what was the impulse behind https://github.com/bitcoin/bitcoin/pull/31768 @brunoerg?

I've tried it on different setups and besides the duration being different as expected for every setup/run I tried I didn't get a case where it's 0. Anyway, I'm fine on removing it or addressing the @hodlinator's sugg
...

💬 hodlinator commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2664114454)
Is the added code protecting some other code? Or just documenting the interface of the RPC, perhaps based on surviving mutants (from mutation testing)? If it's something like the former, I think it should be fixed. If it's the latter, I would lean towards reverting unless people agree we should start testing every aspect of the RPC interface (gradually or not).

📝 brunoerg opened a pull request: "test: remove scanning check on `wallet_importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/31893)
May cause intermittent issues. See: https://github.com/bitcoin/bitcoin/issues/31881

💬 brunoerg commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2664125319)
I just opened #31893 to remove it. It's better to avoid any possibility of intermittent issue.



> Is the added code protecting some other code? Or just documenting the interface of the RPC, perhaps based on surviving mutants (from mutation testing)? If it's something like the former, I think it should be fixed. If it's the latter, I would lean towards reverting unless people agree we should start testing every aspect of the RPC interface (gradually or in larger PR(s)).

It's just a test covera
...

👍 hodlinator approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2621999617)
re-ACK 80e695e6f1edd1a6d9aaab0748a5b573c4492cb4

<details><summary>Tested, looks good.</summary>

```
₿ build/src/bitcoin-cli -regtest loadwallet
error code: -1
error message:
loadwallet "filename" ( load_on_startup )

Loads a wallet from a wallet file or directory.
Note that all wallet command-line options used when starting bitcoind will be
applied to the new wallet.

Arguments:
1. filename (string, required) The path to the wallet directory (absolute or relative to th
...

💬 hodlinator commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2664135447)
> It's just a test coverage addition but not based on surviving mutants since it does not even have test coverage.

Ah, forgot that mutants are focused on modifying test code, not implementation.

💬 brunoerg commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2664137413)
> Ah, forgot that mutants are focused on modifying test code, not implementation.

No, mutants are focused on mutating the implementation to verify the tests. However, if there is no test coverage for the implementation, it does not make sense to modify it, there is nothing to verify.

👍 theStack approved a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2622017122)
re-ACK debacd1f59641c1e9e268c20820f4c8f0fc583f7

Only a [missing valid test vector](https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2664050910) has been added since my previous ACK.

👍 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
...