This makes sense on its own I think - it's similar to another draft PR I had (bitcoin-dev-tools#167). If you think it's related and adds anything, feel free to cherry-pick it here.

This makes sense on its own I think - it's similar to another draft PR I had (bitcoin-dev-tools#167). If you think it's related and adds anything, feel free to cherry-pick it here.

That's cool, will check it in the coming days.
First quick glance it seems that if the mempool is empty; we could avoid calling removeForBlock entirely. Which will save us from adding another callback into the validation queue too, which will leave space for processing another block during ibd.
Yet, that was a fast check. Should check who is using the MempoolTransactionsRemovedForBlock event.

To simplify review, could you split it into several commits - one changing the m_initial_sync(_finished) implementation, one adding it next to role == ChainstateRole::BACKGROUND and one with this change - explaining the context (which are a bit muffled for me currently) in each commit message?

I'm also curious if it's possible to get back to being in IBD after we're finished (e.g. after being offline for weeks, i.e. if (chain.Tip()->Time() < Now<NodeSeconds>() - m_options.max_tip_age) {).

Opened it separately in #32827, please resolve this comment

I find the comment redundant, it states exactly what the code already states clearly, doesn't add any new info

We can safely skip this because we're never requesting single transactions during IBD (i.e. AlreadyHaveTx isn't needed during IBD)?

for (const auto& ptx : pblock->vtx) {
    RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256());
    if (ptx->HasWitness()) {
        RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256());
    }
    m_txrequest.ForgetTxHash(ptx->GetHash());
    m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
}

Can we guard this by some other criteria instead of the IBD boolean, like we're doing with EraseForBlock, e.g.

if (!m_txrequest.Size()) return;

We can safely skip this because we're never requesting single transactions during IBD (i.e. AlreadyHaveTx isn't needed during IBD)?

for (const auto& ptx : pblock->vtx) {
    RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256());
    if (ptx->HasWitness()) {
        RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256());
    }
    m_txrequest.ForgetTxHash(ptx->GetHash());
    m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
}

Can we guard this by some other criteria instead of the IBD boolean, like we're doing with EraseForBlock, e.g.

if (!m_txrequest.Size()) return;

I thought about it but didn't do it on purpose. The reasoning is that we still need to add the txs into the recently confirmed filters post-IBD even when we don't have any tx requests. In other words, we still need to traverse the block. Could at most skip the last two lines with the emptiness check.

Could at most skip the last two lines with the emptiness check

Not sure that helps

is the comment (already mentioning IBD) still accurate?

is the comment (already mentioning IBD) still accurate?

The comment was accurate, but the code didn’t behave as described. Now it does.

there should be a better way to do this than passing around a boolean over so many layers. I'll review it properly this week.

there should be a better way to do this than passing around a boolean over so many layers. I'll review it properly this week.

Did you check the second approach proposed in the PR description?

this is a common-sense change by itself 👍

tiny note: this also skip post-IBD blocks when we don't have any orphans.
--Credits to @mzumsande for pointing it out too--
We could do the same for the tx requests tracker loop but.. that wouldn't change much as we still need to traverse the block for the "recently confirmed" filter anyway.

Shouldn't checking role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload() be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don't think that really matters here. We also use it similarly in the BlockChecked method.

That's why I asked for !m_initial_sync_finished as well.
If the race is biased towards reenabling it too early (rather than later), I agree that it should be fine.

furszy reminded me that BlockChecked is run synchronously, so I don't think that comparison makes sense after all. I guess this comment can be resolved.

Shouldn't checking role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload() be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don't think that really matters here. We also use it similarly in the BlockChecked method.

The diff between BlockConnected() and BlockChecked() is that the former one runs async while the latter runs synchronously. So the idea was to avoid locking cs_main in a different thread which delays other parts of the node.

I also thought about using the 'm_initial_sync_finished' bool for this, but that's not good because the field is calculated at a different time interval in the stale tip check thread (we could compute it more often but that's a different story).

The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).

The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).

I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param

I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I'd vote for that if we really need this param

Ok, I'm a ~0 on this. I think this field is special enough (reason below) to be signaled as a separate arg or it could be wrapped into a struct — similar to how it's done in the interfaces::Chain::Notifications class. With this, I'm not saying to not try the TxDownloadManager enabled flag approach, I just think that the current approach is slightly more useful for future listeners usages.

The only thing I'm somewhat convinced of is that, just like listeners are informed about blocks being downloaded through AssumeUTXO background chain validation, it would be nice if they are aware of IBD blocks — otherwise, sooner or later they will end up re-implementing the same racy/locking isIBD() logic across multiple layers (which could be fine for some cases but it doesn't seem to be generally good).

it would be nice if they are aware of IBD blocks — otherwise [...] they will end up re-implementing

Is there a more reusable way we could implement this?

m_initial_sync_finished will normally be set to true, so this is dead code now. Surprising that no tests failed ...

👍 for using an atomic here!

Note: the usages are always inverted now, can we rather have std::atomic<bool> m_initial_sync{true} instead?

To simplify review, could you split it into several commits - one changing the m_initial_sync(_finished) implementation, one adding it next to role == ChainstateRole::BACKGROUND and one with this change - explaining the context (which are a bit muffled for me currently) in each commit message?

I'm also curious if it's possible to get back to being in IBD after we're finished (e.g. after being offline for weeks, i.e. if (chain.Tip()->Time() < Now<NodeSeconds>() - m_options.max_tip_age) {).

does this introduce any behavioral change? (i.e. m_initial_sync_finished being set to true at a different lifecycle)

Q: is the guard only meant for tests or can this happen in prod as well? Given that we only need initial_sync_finished when this is set, was wondering if it would make sense to guard the initial_sync_finished = !chainman.IsInitialBlockDownload() as well - but not sure if that makes it clearer or more cumbersome. Just documenting my questions, no action needed.

• indicate doesn't hint that we're actually setting it as well
• historical might not be accurate given that we can re-enter IBD mode I think

Could at most skip the last two lines with the emptiness check

Not sure that helps

Given that we're always calling this with tip->GetBlockTime() and converting it to seconds on the call sites, we might simplify it to accept uint64_t block_time and convert to seconds inside instead. Not sure it's better, just noticed a pattern...