💬 maflcko commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1665942681)
79a970d4f12458a175317c453e251db7846c3561: I am not really sure, what the point of `Update` here is. A simple `return Interrupted{};` compiles as well. I fail to see why it would be wrong, because the default constructed monostate should not be returned either way.

(Same for all code below)

My recommendation would be to keep the code as simple as possible and not add complicated features or complication that isn't required.

👍 itornaza approved a pull request: "Stratum v2 Noise Protocol"
(https://github.com/bitcoin/bitcoin/pull/29346#pullrequestreview-2159282833)
Concept ACK a2436955f5bde920e41e03a5db34d477bfeb04a6

As stated in my previous comments, the implementation of the Stratum v2 Noise Protocol is adding more flexibility to Bitcoin than overhead towards the integration of the Stratum protocol Template Provider and Job Declaration Client roles within bitcoind.

I include some really minor nits that came out during a quick code review against the protocol specs and leaving them here to your discretion.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665927305)
nit: Is that comment needed? SV2 spec 4.5.2 is not relevant to message length.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665929102)
styling nit: Consider splitting into multiple lines for clarity and consistency with Serialize above.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665945833)
nit: Should the `ciphertext` message be the @param[out] in the EncryptWithAdd method?

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665944790)
nit: Should the `plain` message be the @param[in] in the `EncryptWithAdd` method?

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665954584)
nit: Consider removing the commented out code. Same to the following line as well.

💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1665952724)
styling nit: Consider dropping the `_our` part from the variable's name to be more consistent with the other names and be closer to the Noise protocol naming convention `s` (implicitly our) and `rs` (explicitly remote). Same for the rest of the names below.

💬 alfonsoromanz commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1666000243)
Thanks @fjahr . I'm open to deleting the third commit. I mentioned this [earlier](https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2086782948) but kept it because I thought my scenario might be somehow different due to the divergent chain.

Your explanation helped me understand the concept of the headers chain, and I think you're right that #30320 may cover the remaining TODOs. I interpreted the part about `"...loading a snapshot when the current chain tip is: [...]"` as referring to
...

📝 theStack opened a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394)
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).

Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net.cpp#L2923-L2930) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](https://github
...

💬 TheCharlatan commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#discussion_r1666030464)
Why is this left as `CC`?

💬 theuni commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#discussion_r1666042736)
Whoops, fixed (along with another typo)

💬 furszy commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1666047097)
Seems that the PSBT 'outputs' field retains the previous output derivation path after replacement. `walletprocesspsbt` appends the new information without removing/updating the previous one. See https://github.com/furszy/bitcoin-core/commit/698e090ead3d0603146c98bd2ec06e44389f34cc.

💬 furszy commented on pull request "Fix cases of calls to `FillPSBT` errantly returning `complete=true`":
(https://github.com/bitcoin/bitcoin/pull/30357#discussion_r1665905291)
nano nit:
```suggestion
psbt = wallet.createpsbt(utxos, [{wallet.getnewaddress(): 0.9999}])
```

💬 TheCharlatan commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1666054400)
Mmh, the footgun here seems less bad than calling `InitScriptExecutionCache` twice, no?

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1666054898)
Thanks for the review @brunoerg. I pushed https://github.com/bitcoin/bitcoin/pull/30243/commits/abe28609c0c7124c8ecf3f09adb537e3b642d50c which adds a test that triggers this error

💬 brunoerg commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1666055522)
Cool

🤔 tdb3 reviewed a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2159508958)
Approach ACK

Great job finding the bug and fixing it.

To sanity check, re-arranged calls to mimick failure recreation (https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200034205). `p2p_handshake` failed as expected after the re-arrange.

```diff
diff --git a/src/net.cpp b/src/net.cpp
index a10be3e33a..6ed167d6fd 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2921,6 +2921,8 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
pnode->
...

💬 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#discussion_r1666085212)
I've re-considered your [preference](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614026697) [above](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1127282385) @jonatack.

💬 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#discussion_r1666089867)
Done.