💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-2110799543)
@theuni any chance you'd be interested in reviewing this? (There are 7 commits so this looks looks like a bigger change, but all the complexity is basically in the first commit, the other commits are small or obvious changes.)

💬 furszy commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1597761901)
nit: unused

🤔 furszy reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-2051516840)
Despite the CI failures, light ACK abbbb1a38d28bf365fb0f219fc60993e48627631.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1600465989)
Done

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1600466113)
Done

💬 hebasto commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2110858446)
> Erm, as of what version?

> I used [this](https://gist.github.com/jamesob/fe9a872051a88b2025b1aa37bfa98605) test case on platforms that support C++20.

... which means, starting from [gcc 9.3](https://packages.ubuntu.com/focal/g++-mingw-w64-x86-64) (Ubuntu 20.04) and onwards.

> Or what fix?

It is hard to say because https://github.com/bitcoin/bitcoin/commit/188ca75e5fe4837d16241446558c7566912f67b2 refers to the mentioned test case only. There was no links to any upstream issues.


...

👍 stickies-v approved a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2053065161)
ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

It took me a while to grok the implications of this PR, but the latest version of this PR makes it much more reviewable and I'm now comfortable that the changes are safe and desired. Also, nice work on the tests!

What I missed/misunderstood in my earlier review is that the reduced orphanage hit rate is a goal, and not a bug/something to minimize. Related nit: perhaps the commit message of 7e475b9648bbee04f5825b922ba0399373eaa5a9 could be explic
...

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598686662)
In c31f148166f01a9167d82501a77823785d28a841:

If this is indeed a refactor commit, I think this line should never be hit. I ran the unit and functional tests with the below diff and didn't get any errors locally. Still, perhaps worth updating the commit like this to make that explicit, and then remove the assertion again in the next commit?

<details>
<summary>git diff on c31f148166</summary>

```diff
diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
index ca8a0e3a92..7132660695 10
...

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1598764600)
tidy nit:
```suggestion
FastRandomContext det_rand{/*fDeterministic=*/true};
```

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600235258)
c++-20 nit:
```suggestion
if (m_orphans.contains(wtxid))
```

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600159941)
In 7e475b9648bbee04f5825b922ba0399373eaa5a9:

I think having the `if/else` path in this commit is helpful, since it distinguishes between when we cast the txid and when we don't.

nit: I think this already works pretty well, but some bits (e.g. "guess what the wtxid is") are not super clear imo. I've summarized my understanding into the below, feel free to use what you like.

```
// Never query by txid: it is possible that an existing transaction in the orphanage has the

...

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600235908)
c++20-nit:
```suggestion
return m_orphans.contains(wtxid);
```

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600337905)
nit: I first interpreted that as that an input had been removed from the transaction, perhaps better to use "parent" as to stay consistent with the language in the next steps?
```suggestion
# 1. Fake orphan is received first, the parent is not yet broadcast.
```

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600252935)
In 8923edfc1f12ebc6a074651c084ba7d249074799:

nit: I feel like documentation on why `TxOrphanage` indexes by wtxid/allows multiple versions of the same transaction should be documented in `TxOrphanage` instead of here.

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600356156)
nit: would `tx_middle_bad_wit` be more consistent naming here?

💬 itornaza commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#issuecomment-2110867498)
trACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600509299)
done

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600510097)
Agree. Not only for error-checking, we'd also expect users to do something with the `FlatFilePos` that `FindNextBlockPos` now returns.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600512963)
Sorry, missing the context here, is this suggesting a change?

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1600513102)
fixed