💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661399034)
> Other than that, what if instead of creating this new `LegacyDataSPKM::AddKeyPubKeyInner` method you place this three lines inside the `LegacyDataSPKM::LoadKey` which is only called during load?

We can't do that because `LoadKey` needs to call `LegacyScriptPubKeyMan::AddKeyPubKeyInner` for legacy wallets that are being loaded normally. Since migration cannot do all of the things in `LegacyScriptPubKeyMan::AddKeyPubKeyInner`, we need to have `LegacyDataSPKM::AddKeyPubKeyInner`.

> I think
...

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661402030)
Done

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661402170)
Removed the assert and the comment.

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#issuecomment-2200765833)
Reordered the commits and preserved the `IsMine` asserts.

💬 mzumsande commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200786670)
since the failures are frequent and it seems that the solution won't be all that trivial, I think it might be best to revert https://github.com/bitcoin/bitcoin/pull/30362 temporarily and re-introduce it together with a fix in `net` - without any pressure to get the CI green quickly.

📝 theStack opened a pull request: "Revert "test: p2p: check that connecting to ourself leads to disconnect""
(https://github.com/bitcoin/bitcoin/pull/30374)
As suggested in https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200786670, this PR reverts the recently added test #30362 that causes frequent CI failures. A TODO is added in the functional test file to re-add it later when the race condition is fixed.

💬 theStack commented on issue "ci: failure in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200816828)
Thanks for the suggestions @vasild and @mzumsande, moving the `PushNodeVersion` part out of `InitializeNode` and calling it after the `CNode` instance is added to the list sounds like a good idea.

> since the failures are frequent and it seems that the solution won't be all that trivial, I think it might be best to revert #30362 temporarily and re-introduce it together with a fix in `net` - without any pressure to get the CI green quickly.

Agree, opened a revert PR #30374.

🤔 mzumsande reviewed a pull request: "Revert "test: p2p: check that connecting to ourself leads to disconnect""
(https://github.com/bitcoin/bitcoin/pull/30374#pullrequestreview-2152087483)
utACK 9ec2c53701a391629b55aeb2804e8060d2c453a4

👍 brunoerg approved a pull request: "Revert "test: p2p: check that connecting to ourself leads to disconnect""
(https://github.com/bitcoin/bitcoin/pull/30374#pullrequestreview-2152094260)
utACK 9ec2c53701a391629b55aeb2804e8060d2c453a4

💬 pinheadmz commented on pull request "Permit opt-out of finalization during `walletprocesspsbt`":
(https://github.com/bitcoin/bitcoin/pull/30357#issuecomment-2200845019)
concept ACK

Confirmed the bug on master, fixed by the patch in this PR.

I'm not sure responding to the user-provided `"finalize"` option in this way is entirely correct though. It seems to me that the `"complete"` value that we return could be inaccurate even before #28414 because we haven't called `PSBTInputSignedAndVerified()` yet. So that makes me wonder if we should be verifying in `FillPSBT()` here, instead of just checking that *something* is in the scriptsig / witness:

https://gi
...

💬 achow101 commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2200850676)
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce

🤔 furszy reviewed a pull request: "#27307 follow-up: update mempool conflict tests + docs"
(https://github.com/bitcoin/bitcoin/pull/30365#pullrequestreview-2152124441)
utACK 7d55796c530

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2200887728)
> > Do blocks mined with the exception count towards the total work according to difficulty-1 or the actual difficulty? If it’s the latter, in testnet3 you could just invalidate the last block in the previous difficulty period with a difficulty-1 block. If it’s the former, I agree, the absence of this attack may indicate that I’m worrying too much.
>
> (Side-note: I first read difficulty-1 as "difficulty minus 1" and got a bit confused but I think you mean the same as "difficulty=1")
>
> I
...

🤔 achow101 reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2152124552)
Concept ACK

I'm not a huge fan of all the boilerplate and wrappers that this ends up adding, but I also don't see a better way to do this.

In `ApplyMigrationData`, it looks like we're creating batchs for the watchonly and solvable wallets multiple times. Those could be consolidated in this PR as well.

💬 achow101 commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1661464266)
In 4e72821e1961b58559f49a4746bc8d15d116f6b7 "wallet: ApplyMigrationData, group all db txs into a single atomic operation"

This could be its own commit.

💬 achow101 commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1661483289)
In faa9a1fcd05341c1cbe9b2e2257c1c0248b2a4d4 "rpc: Avoid getchaintxstats invalid results"

typo: "start end end". I think that's supposed to be "start and end"

```suggestion
"Only returned if \"window_block_count\" is > 0 and if txcount exists for the start and end of the window."},
```

✅ ceffikhan closed a pull request: "test: fix debug log assertion in p2p_i2p_ports test"
(https://github.com/bitcoin/bitcoin/pull/30345)

💬 ceffikhan commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200940397)
closing PR for now

💬 paplorinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200977622)
> Ok, the reason for https://github.com/bitcoin/bitcoin/pull/7656#issue-139438690 was an improvement in listunspent. Seems fine, if this is still the case. But this will need to be checked first.

I've created 10k blocks in regtest and measured the performance of `listunspent` before and after:

```bash
killall bitcoind 2>/dev/null || true
./src/bitcoind -regtest -daemon
./src/bitcoin-cli -regtest createwallet "test_wallet"
./src/bitcoin-cli -regtest generatetoaddress 10000 $(./src/bitco
...

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2200986730)
> Also, "protocol.h" in the first commit message body seems wrong or out-of-date here: `bringing the error message in line with the definition established in protocol.h ("error when there are multiple wallets loaded")`. I'm not sure, but did you mean "src/wallet/rpc/util.cpp" instead?

The reference to "`src/rpc/protocol.h`" was intended to clarify the context of the error message alignment, specifically in the section of the code where the RPC error code "not wallet specified" is defined (`en
...