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

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

💬 andrewtoth commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2209627023)
Just a style nit, but I think if you return early on `if (!inserted)` it would make a cleaner diff and make it easier to see the benefit of this change.

```diff
diff --git a/src/coins.cpp b/src/coins.cpp
index a4e4d4ad32..9e8072ba02 100644
--- a/src/coins.cpp
+++ b/src/coins.cpp
@@ -41,20 +41,20 @@ size_t CCoinsViewCache::DynamicMemoryUsage() const {
}

CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const {
- CCoinsMap::iterator it = cacheCoins.find(o
...

💬 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-2209638547)
Updates:
- Addressed latest @jonatack's [feedback](https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-2078072832):
- Removed [unnecessary](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614031474) trailing `.` from CLI error messages.
- Reconsidered previous [suggestion](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614026697) about the `RPC_WALLET_NOT_SPECIFIED` error handled in `src/bitcoin.cli` and the returned message.
- Added [full error messa
...

💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#discussion_r1666119166)
I didn't do that because there are already multiple files that use it (`validation_chainstatemanager_tests`, `validation_chainstate_tests`). Seems less likely that the same would happen for fuzz tests.

💬 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-2209660068)
Updates:
- Addressed latest @jonatack's [feedback](https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-2078072832):
- Removed [unnecessary](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614031474) trailing `.` from CLI error messages.
- Reconsidered previous [suggestion](https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1614026697) about the `RPC_WALLET_NOT_SPECIFIED` error handled in `src/bitcoin.cli` and the returned message.
- Added [full error messa
...