💬 achow101 commented on pull request "build, test: Build `db_tests.cpp` regardless of `USE_BDB`":
(https://github.com/bitcoin/bitcoin/pull/31617#issuecomment-2576143826)
ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1905991017)
Done

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1905991117)
Removed

💬 ryanofsky commented on issue "multiprocess: `ipc_tests` fail on NetBSD":
(https://github.com/bitcoin/bitcoin/issues/31618#issuecomment-2576154238)
I would guess based on the `std::system_error` "Invalid argument" error this failure should probably happen reliably instead of randomly, but hard to know.

It seems like from the logs `IpcPipeTest` is succeeding and the error is probably coming from one of the following tests `IpcSocketPairTest` or `IpcSocketTest` even though those tests do not appear to be printing anything. Maybe the reason they aren't printing anything is because they are using [`LogDebug`](https://github.com/bitcoin/bitco
...

💬 davidgumberg commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2576159435)
trivial reACK https://github.com/bitcoin/bitcoin/commit/352391c2cf1a45231ae92ca92d2415b3786ab9ad

Recent push only changes comments.

<details>

<summary>
git diff 815467f..352391c
</summary>


```diff
$ git diff 815467f..352391c
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 04e487f884..4f5c38bb7b 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -186,7 +186,7 @@ using Opcode = std::pair<opcodetype, std::vector<unsigned char>>;
templa
...

🤔 marcofleon reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2535162556)
Code review ACK 86d7135e36efd39781cf4c969011df99f0cbb69d

Ran all the relevant tests, including the fuzz tests for about 50 cpu hours each. The implementation here of keeping track of multiple peers that announce an orphan looks good to me. It makes sense to not be locked in to only one peer for orphan resolution, as that peer could disconnect (in which case we lose the orphan) or be malicious.

Just left some non-blocking nits below.

Also quick question for my own understanding. Is it tr
...

💬 marcofleon commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905930558)
In the loop below this, should it be `const Txid& parent_txid`? However, if this is more rework of this function than you're looking to do (adding multiple `ToUint256`), then seems fine to leave as is for now.

💬 marcofleon commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905960235)
nit: This doesn't use any member variables of `TxDownloadManagerImpl` so it could be just a standalone helper function in `txdownloadman_impl.cpp`.

💬 marcofleon commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905920861)
Took me a bit to figure out how this could happen, so a comment might be helpful here.

👍 instagibbs approved a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2535269451)
reACK 86d7135e36efd39781cf4c969011df99f0cbb69d

feel completely ok to ignore all my comments

Major change is recomputing the missing parents per announcement of orphan, rather than a static list for the lifetime of the orphan when first received, and also correctly accounting for those missing parents when deciding when to rate-limit the announcements for a specific peer..

💬 instagibbs commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905994963)
```Suggestion
assert parent_peekaboo_AB["txid"] not in node.getrawmempool()
assert tx_replacer_BC["txid"] not in node.getrawmempool()
```

💬 instagibbs commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905989913)
```Suggestion
const auto wtxid{Wtxid::FromUint256(gtxid.GetHash()))};
if (auto orphan_tx{m_orphanage.GetTx(wtxid)}) {
```
and use `wtxid` below in the other two locations

💬 instagibbs commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1905999581)
```Suggestion
# Second peer is an additional announcer for this orphan with different missing parents than prior announcement
```
micro-nit something like?

💬 instagibbs commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1906001592)
think this should work too?
```Suggestion
node.bumpmocktime(TXREQUEST_TIME_SKIP)
```

💬 mzumsande commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2576178033)
I tried to reproduce the crash locally (putting a sleep into AttachChain) but didn't manage - Not the biggest GUI or wallet expert so I may well be missing something, but here's what I tried:
- With qt, how do I create more transactions while the wallet is loading? The GUI blocks for me until the wallet is loaded.
- With bitcoind, I was able create more blocks during the rescan - but those will create `blockConnected()` notifications, and several RPCs that call `GetTxBlocksToMaturity` (such as
...

💬 instagibbs commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2576185775)
@marcofleon

>Also quick question for my own understanding. Is it true that in the honest case, orphan resolution will probably happen with the first peer that announced it to us?

It Depends(TM). If the first announcer was an inbound connection and second was an outbound connection less than 2(?) seconds later, the outbound will be chosen first. see the functional test case `test_orphan_handling_prefer_outbound`

💬 fjahr commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#issuecomment-2576200328)
re-ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b

💬 kevkevinpal commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#issuecomment-2576244717)
ACK [1ea7e45](https://github.com/bitcoin/bitcoin/pull/31462/commits/1ea7e45a1f445d32a2b690d52befb2e63418653b)

💬 mzumsande commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1906056946)
I really wasn't sure what the best solution is when I opened the issue and still am not, but these are my thoughts:

Yes, there are two reasons why we might not apply assumevalid during a reindex currently:
1.) The best header has not enough chainwork
2.) The assumevalid block is not in our block index

> I could change this to always skip script checks during a reindex, but IIUC having a block on disk doesn't mean that it was connected during the previous IBD.

I think the question is
...

💬 sipa commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1906077380)
Something like this?

```c++
bool TxDownloadManagerImpl::DoOrphanThingRenameMe(const std::vector<Txid>& unique_parents, const Wtxid& wtxid, NodeId nodeid, std::chrono::microseconds now)
{
if (auto delay{OrphanResolutionCandidate(nodeid, wtxid, unique_parents.size())}) {
const auto& info = m_peer_info.at(nodeid).m_connection_info;
for (const auto& parent_txid : unique_parents) {
m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(parent_txid), info.m_preferred, n
...