💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2200737355)
Code review ACK a05f4ca241045e8a4b295760590805f961c2e5e7.

Just changed psbt error string and fixed a test since the last review.

👍 pinheadmz approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2152010530)
ACK ffe9b274984a351716348a6c99df19c391bfdc8e

Re-reviewed changes since last ACK, including addition of `tor_port()` in tests which I like.

Also ran my extra test again from https://gist.github.com/pinheadmz/0bea4bf8d8bf9af85f8ae326286d3082

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK ffe9b274984a351716348a6c99df19c391bfdc8e
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmaC8jkACgkQ5+KYS2KJ
yT
...

💬 willcl-ark commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-2200759744)
Concept ACK.

I was tacking this today (missed this PR as GH search for "sse" yielded no results 🤦🏼‍♂️ )

First I tried re-implementing @luke-jr previous approach, but refactoring into macros: https://github.com/bitcoin/bitcoin/commit/cdaa9bfd6e63dc675cdd27e6d586f69423ee1748

Whilst this works well-enough, it didn't particularly fix https://github.com/bitcoin/bitcoin/issues/28864 in my opinion, so I tried https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:detect-sse4.
...

💬 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)