💬 glozow commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#issuecomment-1623886758)
Investigating, thanks @DrahtBot

💬 furszy commented on pull request "test: Restore unlimited timeout in IndexWaitSynced":
(https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623901376)
> > Which could happen on all the index sync errors.
>
> Good point. Though, that seems like a bigger problem of the unit tests not handling AbortNode/FatalError/StartShutdown at all?

The quickest fix would be to add a `ShutdownRequested()` call into the `IndexWaitSynced()` loop. And verify at the end of the function that the index class is synced by calling:
```
BOOST_CHECK(index.GetSummary().synced);
```

Still, the "AbortNode/FatalError/StartShutdown" treatment on the tests topic s
...

💬 achow101 commented on pull request "wallet: fix bug, simplify and add coverage for addressbook migration":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1623901650)
This PR ends up doing a lot of different things that are only tangentially related. I think it could be split up into a few smaller PRs, e.g. one for the migration fix, one for the migration refactoring, and one for the other refactors.

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1623911361)
`28d26c4f37...20b49460b3`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253688687

Invalidates ACK from @jonatack

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1254628447)
Looks better, thanks!

🤔 furszy reviewed a pull request: "wallet: fix bug, simplify and add coverage for addressbook migration"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1516852781)
> This PR ends up doing a lot of different things that are only tangentially related. I think it could be split up into a few smaller PRs, e.g. one for the migration fix, one for the migration refactoring, and one for the other refactors.

sure, will work on it.

🤔 instagibbs reviewed a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1515046495)
some initial comments through https://github.com/bitcoin/bitcoin/pull/28031/commits/116378efc1c9c1fe0d26cb42e2bdbb5770815c35

Log changes suggested are helpful for tracing what's happening in the orphanage on my node I'm testing.

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253443521)
make these be TXPACKAGES? then you get the entire "story" with a single log type (which helped me diagnose the `Timeout` issue)

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253454308)
`m_orphan_request_tracker` :pray:

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253457139)
seems wrong/very noisy without this? e.g., this line https://github.com/bitcoin/bitcoin/pull/28031/files#diff-ece439372a3e31da3141ed8fda99b37381e32cdab17ca26fffd5dfd916c300c8R124 will fire constantly

```suggestion
m_orphanage.EraseTx(ptx->GetWitnessHash());
orphan_request_tracker.ForgetTxHash(ptx->GetWitnessHash());
```

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253459892)
any principle on prefixing and postfixing `\n` to everything in these logs?

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253461532)
```suggestion
LogPrint(BCLog::TXPACKAGES, "\nForgetting orphan request %s from peer=%d\n", gtxid.GetHash().ToString(), nodeid);
```

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253619584)
since we're using `size_t` for these fields, do we want to continue on with UB for release builds or do an assert?

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253479136)
trying to fetch after failure seems wrong

```suggestion
m_orphanage.EraseTx(wtxid);
orphan_request_tracker.ForgetTxHash(wtxid);
```

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253623061)
> [txorphanage] handle AddTx(nullptr)

Could we motivate this change in the commit message?

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253643847)
Suggested comment, something like:

"We do not presume the parent will still be in the orphanage by the time a response is received, so we exclude the orphanage from the check when deciding what to request."

If that's wrong, then it needs better explanation than what exists in the commit message :)

💬 RandyMcMillan commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1623991042)
Concept ACK

👍 ryanofsky approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1516827895)
Code review ACK 30b2511f39d32e29f9f05859aa8a97b84c22376b. Just
`GetFirstStoredBlock` and `CheckBlockDataAvailability` related improvements since last review.

It would be nice to have a third reviewer for PR this if anyone else wants to take a look.

---

re: https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254460179

> maybe would be good to discuss further what "pruneheight" result should be in a follow-up. it sounds better if we directly return the last pruned height.

I
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254618240)
In commit "refactor: simplify pruning violation check" (9ef5099dac06f0cc4b904fefe9620c9fc09c0ddc)

Would be probably make sense to move new this comment to the earlier commit "make GetFirstStoredBlock assert that 'start_block' always has data" (79449741fa7b2fc3e59dbc92e408cff547a885fa), since everything described is true there and applies more to that commit.

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254647941)
In commit "index: verify blocks data existence only once" (30b2511f39d32e29f9f05859aa8a97b84c22376b)

The variable name `need_start_from_scratch` seems misleading because it will be true even when not syncing from genesis. Would suggest simplifying and clarifying with:

```diff
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1892,9 +1892,6 @@ bool StartIndexBackgroundSync(NodeContext& node)

ChainstateManager& chainman = *Assert(node.chainman);
for (auto index : node.indexes) {
-
...