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

💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#discussion_r1666119672)
fixed, the "," went into the hash by mistake (and was ignored). I also fixed the `hash_serialized`, which was wrong somehow.

💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#discussion_r1666119718)
fixed, here and at one more spot.

💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#discussion_r1666119744)
Done!

💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#discussion_r1666119817)
Done, use `*3`

💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2209669755)
Thanks for the reviews! I addressed the feedback by @maflcko and also corrected `hash_serialized` in the chainparams. Not sure why it was wrong before, I'm pretty sure I was able to activate the snapshot before opening the PR on another computer. Probably just an oversight, but maybe someone can confirm that everything is deterministic and the snapshot can be loaded now (e.g. by adding an assert and setting `base_blockheight` and `m_coins_count` to 200 it should crash almost immediately).

> I
...

💬 chennleyi commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2209881209)
Hi @murchandamus, is this issue about to get resolved? can I work on this issue? I am new to this.