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

💬 mzumsande commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + tests":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1666308257)
I think that "chain tip" usually refers to blocks we have connected to a chainstate (so headers-only would not qualify), so I would suspect that the original intention of the TODO was just what the third commit implements.

But I agree with @fjahr that it doesn't really add too much to `test_snapshot_with_less_work` - less work is less work, no matter if the chain is divergent or not. Plus, I agree that if #30320 gets merged it would become a special case of that, because a chain tip with more
...

💬 maflcko commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#discussion_r1666348629)
> the "," went into the hash by mistake (and was ignored)

ugh. I didn't even notice this

💬 maflcko commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2210242534)
re-ACK de71d4dece604907afc4fc26b7788e9c1a4cbecb 🎆

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK de71d4dece604907afc4
...

💬 paplorinc commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2210308399)
Thanks for the review @andrewtoth.
I'm not sure I see the advantage of inverting the `if (inserted) {` call, just to have two `return it;` calls after.
But I did rename `it` to `ret`, since we're getting the most likely return value immediately now.