💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191251631)
Would this be better as a `map<uint256, map<NodeId, list<>::iterator>>` ? Seems like it'd make some of the `FIXME`s easier.

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191307770)
I think this case is (mostly) for when a non-high-bandwidth peer tells us about a block, we request it via a compact block, and then we get a different block from some other peer, UpdateTip to that, and only afterwards see the CMPCTBLOCK response from the original peer. But now that response is probably useless, so replace it with a GETDATA.

I wonder if that might be better expressed as: `if (in_flight_same_peer && pindex->pprev != ActiveChain().Tip())` ?

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191313364)
Might be better to name this `requested_block_from_this_peer` ? (I think for new code we prefer `bool x{false};` so we get errors if we're accidently narrowing values?)

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191357478)
Out of the last 612 compact blocks my node's reconstructed that required requesting transactions, 277 needed 10 or fewer (45%), 441 needed 50 or fewer (72%), 488 needed 100 or fewer (80%), 548 needed 500 or fewer (90%). I'd probably prefer 50 or 100 than 10, I guess?

(In simpler times, 1086/1162 (93%) need 10 or fewer, 1140/1162 (98%) needed 50 or fewer, 1144/1162 (98.5%) needed 100 or fewer. Again, I'm not including blocks that requested 0 txs in those denominators)

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191323390)
(Test the bool first?)

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191320979)
`already_in_flight == 0 || (already_in_flight == 1 && in_flight_same_peer)` ?

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1191369251)
This logic is different to the previous `first_in_flight`; probably should be the same?

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#discussion_r1191393064)
Lots of magic numbers in this test. Could you assert specific vsizes on each tx, and have that number match the picture here?

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#discussion_r1191360507)
test/txpackage_tests.cpp:382:9: warning: multi-line comment [-Wcomment]
382 | // / \
| ^

💬 instagibbs commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#discussion_r1191354472)
`grandparent_inputs.size() * COIN`

🤔 mzumsande reviewed a pull request: "Improve performance of p2p inv to send queues"
(https://github.com/bitcoin/bitcoin/pull/27610#pullrequestreview-1422952009)
Post-Merge ACK.

I've been running this patch (plus added reporting in `getpeerinfo`, https://github.com/mzumsande/bitcoin/commit/6932997b1b41d2197094a0a9c71601df72630501) for a few days, and have neither seen extremely high backlogs > 5000 in `m_tx_inventory_to_send` nor enhanced CPU use.

In order to better monitor the efficacy of this, it might make sense to expose the size of `m_tx_inventory_to_send` via RPC permanently?

💬 ajtowns commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#discussion_r1191396005)
I don't think the timing of removal from `mapRelay` reveals a secret -- that's 15minutes after it was added, but the time it was added is the time it was first announced (probably to an outbound peer), which is already randomised/batched, compared to the time we received it?

💬 vasild commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1544261088)
> > `txinfo.m_time <= mempool_req`

> This condition is checking that our inv (`txinfo`) is more recent than the last `mempool_req`, meaning that once a mempool request has been processed the peer can request any transaction from our mempool.

I don't get it. Is it not that the condition is checking whether `txinfo` is _older_ than the last `mempool_req`? In that case the transaction has been part of the `MEMPOOL` reply, so we have sent an `INV` about it.

💬 ajtowns commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544274997)
FWIW, I've thought about moving `mapRelay` into txmempool so that its size can be counted as part of the 300MB (or whatever), and, if it grows too large, that we could trim `mapRelay` before their scheduled expiry time has actually been reached. Then you could have a multi_index amongst `txid`, `wtxid`, `expiry` to an `CTransactionRef`, along with a counter of how many entries in `mapRelay` aren't also in the mempool for handling memory usage?

🤔 ryanofsky requested changes to a pull request: "init: verify blocks data existence only once for all the indexers"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1422846165)
Reviewed 86752e0cc5bc48f3d4ac1cd07835c37daf078d6a and it mostly looks good, but does have a null pointer deference currently (see comments and suggested fix). I like this PR because it will simplify #24230 and could potentially improve performance. I'm not actually how sure how much it actually would improve performance in practice though, so I'm curious if that's really the main motivation here or if this is related to one of your other changes. I'm also curious about the todo:

> Pending tod
...

💬 ryanofsky commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1191390577)
In commit "indexes, refactor: Remove index prune_violation code" (1341793b928ba81205b1cea11fdbd52fe6c3e869)

Note for reviewers: code in this function is moved from the BaseIndex::Init function with minor changes. An easy way to review it is to look at the before and after versions in a side by side diff.

💬 ryanofsky commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1191383733)
In commit "index: verify blocks data existence only once" (86752e0cc5bc48f3d4ac1cd07835c37daf078d6a)

In the case where the bitcoind datadir is being newly created and the `LoadChainState` call above doesn't set a chain tip, `pindex` will be null here and this line will segfault. It should be possible to handle this with `if (!pindex) return`. This is causing CI errors (https://cirrus-ci.com/task/4539645451567104) currently

💬 ryanofsky commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1191329811)
In commit "index: verify blocks data existence only once" (86752e0cc5bc48f3d4ac1cd07835c37daf078d6a)

Not important in practice, but since `m_best_block_index` is an atomic variable, it would look a little better to retrieve it atomically with:

```
if (const CBlockIndex* pindex = m_best_block_index.load()) {
```

💬 ryanofsky commented on pull request "init: verify blocks data existence only once for all the indexers":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1191396717)
In commit "indexes, refactor: Remove index prune_violation code" (1341793b928ba81205b1cea11fdbd52fe6c3e869)

It would probably make more sense to make this method into a standalone function `bool ChainDataFromTipDown(ChainstateManager& chainman, const CBlockIndex& start_block)` function in someplace like `src/node/chainstate.cpp` than to add it to interfaces::Chain if it won't be called by chain clients like wallets or indexes.

💬 hebasto commented on pull request "wallet: fix deadlock in bdb read write operation":
(https://github.com/bitcoin/bitcoin/pull/27556#issuecomment-1544286558)
Concept ACK.