Nit, typo: "before being given DoS points" ?

Done as part of #25555

Nit: braces are unnecessary, I think.

Done as part of 8efa73e

7f24508 It might be good to mention in the commit message that you are changing the points in this "move" commit.

Previously a non-connecting non-continuous header sequence of size MAX_BLOCKS_TO_ANNOUNCE+1 was assigned 20. Now it is assigned 10, no?

It still gets a score of 20 via HandleFewUnconnectingHeaders() when nCount <= MAX_BLOCKS_TO_ANNOUNCE; this adds an unavoidable score of 10 for unconnecting headers when nCount > MAX_BLOCKS_TO_ANNOUNCE which previously was more or less ignored...

Previously a non-connecting non-continuous header sequence of size MAX_UNCONNECTING_HEADERS would be submitted to validation through ProcessNewBlockHeaders(), since the if (!m_chainman.m_blockman.LookupBlockIndex(headers[0].hashPrevBlock) && nCount <= MAX_BLOCKS_TO_ANNOUNCE) block would be skipped.

That would return BlockValidationResult::BLOCK_MISSING_PREV:

which would result in 10 misbehaviour points in MaybePunishNodeForBlock

this adds an unavoidable score of 10 for unconnecting headers when nCount > MAX_BLOCKS_TO_ANNOUNCE which previously was more or less ignored...

Can you explain a bit more why this was "more or less ignored"?

Previously it was deterministically and consistently assigned a Misbehaving(peer, 20, "non-continuous headers sequence");.

Now it is deterministically and consistently assigned a Misbehaving(peer, 10, "invalid header received");.

Previously a non-connecting non-continuous header sequence of size MAX_UNCONNECTING_HEADERS would be submitted to validation through ProcessNewBlockHeaders()

No, it wouldn't, as it would be rejected by the Misbehaving(peer, 20, "non-continuous headers sequence"); check?

What am I missing?

What am I missing?

Nothing. I misread non-continuous as continuous. You're right that this would be rejected with Misbehaving(peer, 20, "non-continuous headers sequence")

Not sure what to say here other than that yes, there are minor behavior changes in this PR. I don't think any are significant as the DoS points here are pretty arbitrary and when checking for different failures, it shouldn't matter too much which order we do things. I would have been happy to update the commit message as you suggest but now that this is merged there's not much I can do!

It should be the case that any failure before that resulted in DoS points would still result in DoS points now, I think -- if that is not the case, then that would be an oversight to fix. The motivation for making these kinds of minor changes is to make the headers processing easier to reason about, which is also in preparation for another change I'll be proposing to headers sync in the future (which I think will be easier to understand with the headers processing logic that is introduced in this PR).

Yeah, it wasn't meant as a criticism of 7f24508 more as a nit to keep in mind in the future. I think the code changes in this commit are perfectly fine. Generally I just prefer to split behaviour changes from refactoring changes, or at least explain the behaviour changes in the commit message briefly. Otherwise, what seems obvious to the author is easily missed by reviewers. (I think this discussion is supporting evidence enough).

Previously a non-connecting non-continuous header sequence of size MAX_BLOCKS_TO_ANNOUNCE+1 was assigned 20. Now it is assigned 10, no?
Previously it was deterministically and consistently assigned a Misbehaving(peer, 20, "non-continuous headers sequence");.

Ah, sorry, I was only thinking of the non-connecting continuous case, not the non-connecting, non-continuous case.

Is there even a need to make this atomic? In a single threaded environment this is not needed. And if the same peer is served by several threads (for whatever reason) in the future, an atomic may prevent UB, but doesn't seem sufficient to prevent races logically.

Hmm that is an interesting point. My sense was that we don't want to rely on implicit single-threadedness to reason about data races, so it was safer to just throw this into an atomic and not worry about it.

I also think that having multiple threads servicing the same peer would not really work for a bunch of reasons (including the one you give, about logical errors in code like this). I would imagine that multithreading network handling in the future would involve different threads servicing different peers.

I think the most likely form of UB from data races would be if we were to expose this value via RPC in the future. So to make this future-proof, I'd prefer that we leave this as-is; it doesn't seem like the performance hit of having an atomic here (which is rarely accessed, as getheaders messages are not a frequent occurrence, and there are far more effective ways to CPU DoS a node than to trigger a getheaders) is significant enough to warrant making this harder to reason about in the future.

I think if this was ever exposed on RPC, we'd also want to think which other fields to return atomically in the response, in which case a per-field atomic isn't enough to prevent logic races. So to me it seems slightly preferable to leave the design of thread safety to when it is needed. Otherwise it may be assumed in the future that this is already perfectly thread safe and can be used as is. Leaving this plain (without atomic) would at least have a thread sanitizer failure hint that the design needs to be re-thought if this was accessed by more than one thread in the future.

(Other than that, agree that atomic doesn't affect performance)

You know, I think you're right that this atomic is incorrect. We need to hold a lock throughout MaybeSendGetHeaders() because this variable can be accessed in the scheduler thread or in the net processing thread, and if we don't hold a lock throughout that function we can get a data race. Thanks for asking about this, I'll open a PR to fix.

"in-thread" means running in the thread "ThreadMessageHandler (b-msghand): Application level message handling (sending and receiving). Almost all net_processing and validation logic runs on this thread."

Right, m_last_getheaders_timestamp is accessed from PeerManagerImpl::MaybeSendGetHeaders() and PeerManagerImpl::ProcessMessage() which are only called from a single thread ThreadMessageHandler().

because this variable can be accessed in the scheduler thread or in the net processing thread

@sdaftuar, I think it is not accessed from the scheduler thread?

I am in favor of simplicity, in this case meaning to not protect data that is accessed by a single thread. If in the future it is accessed by another thread, then an appropriate protection should be added. That may be an atomic or a mutex just for that variable or a mutex for multiple variables depending on the need. Otherwise we may end up protecting it needlessly now and later to have to rework the protection anyway (or worse - use the available, inappropriate protection, like @MarcoFalke mentioned above "...it may be assumed in the future that this is already perfectly thread safe...").

Can you elaborate a bit on this? Unless I am missing something MaybeSendGetHeaders/ConsiderEviction is only called in-thread, not to be confused with CheckForStaleTipAndEvictPeers, which is called out-of-thread. What am I missing?

Ah, I guess I misremembered how ConsiderEviction is called (and thought it was run in the scheduler thread).

I'll update my other PR and drop the mutex/atomic altogether.

I am in favor of simplicity, in this case meaning to not protect data that is accessed by a single thread.

Possible idea for the future. In theory could have SingleThread<T> class similar to the Synced<T> class from #25390 that wraps an object and asserts in debug mode that is always accessed from the same thread. I think a solution like that would be overkill here, and I'm struggling to think of cases where it wouldn't be overkill, but it wouldn't be hard to implement and could be useful to help code document assumptions it is making.

Should be resolved by 8efa73e

msgMaker is now unused in ProcessHeadersMessage. This is fixed in #25717