Optimize when to acquire ledgers from the network.

[ximinez]

Particulary avoid acquiring ledgers likely to be
produced locally very soon.

Derived from XRPLF#4764

This file compares the differences between the changes in XRPLF#4764 and this one: diff.txt . The most notable differences are:

• Adds more logging to InboundLedgers::acquire, including the decision for the old logic, and messages for the other "Abort" conditions.
• InboundLedgers::acquire accounts for the request type. Specifically, if it's for CONSENSUS, and the node is otherwise keeping up, it will not attempt to get the ledger.
• More logging in NetworkOPsImp::beginConsensus
• Reverses the "falling behind" logic, which was detecting if we were running ahead in the other version.

---

This change is 

[mtrippled]

Please comment the reasoning for each of those things I described, and consider a lambda or some way to make the shouldAcquire check more intuitive to the reader, if at all possible.

Also, think through the boolean logic there. I think it's correct, and hopefully it tests well.

Also, consider the possible implications if it is indeed correct as written. The only one I can think of is if we are falling behind the network but are less than 20 ledgers behind, we won't try to acquire. Maybe that can be tuned. Then again, if acquiring ledgers is potentially expensive, perhaps it burdens us. But that's the main consideration I think. I think the threshold should be something whereby we definitely are behind the network and need to jump ahead to the latest ledger. I think 20 is fine that way, but I think those are the considerations for it.

[mtrippled]

I think this logic is OK, but I suggest putting comments and/or cleaning up a bit as follows:

Namely, nearFuture is if:
Requested seq > valid seq BUT requested seq is < 20 greater than the valid seq. It means the requested seq is in the near future relative to the validated ledger. The requested is is not behind or even, but between 1 and 19 inclusive ledgers ahead of the valid ledger. Therefore, I shouldn't be trying to acquire this ledger. I'm not there yet, and it's possible/likely I have the tx's necessary to build it if I ever need to, and it might not even become validated. On the other hand, if it's 20 or so in the future, I should request it so that I can jump ahead because it probably means I'll have a hard time catching up otherwise.

fallingBehind is if the last closed ledger is at least 2 behind the validated ledger. I am falling behind the network. The reason this should not simply be only at least 1 behind the validated ledger is that I think a slight lag is a normal case because some nodes get there slightly later than others. But a difference of 2 means that at least a full ledger interval has passed, so we are beginning to fall behind.

We shouldn't be acquiring this ledger if we're full. Assume we are functioning correctly and have the necessary tx's for when the time comes.

So, here's what I think the line is checking for. Namely, acquire the ledger if any of the following are false:
1) We're in sync, or
2) we are not falling behind, or
3) the request is for a ledger in the near future, or
4) it's a Reason::CONSENSUS request.

Please confirm the reason for checking the ledger request type Reason::CONSENSUS.
As written, I think it means to always acquire an inbound ledger if it's for something other than CONSENSUS. In other words, only suppress CONSENSUS ledgers (if other conditions are met).

Since this is all boolean math, it easily could be the opposite of everything I just wrote if 1 thing is wrong.

It might make sense to re-write this line to something much easier to reason about, for future generations, after comments like I've described.

Perhaps something like a lambda with this logic:

if not full:
return true
if falling behind:
return true
if request for ledger NOT in near future:
return true
if NOT a Reason::CONSENSUS ledger:
return true

return false

---

[ximinez]

The logic you outlined isn't quite right, but I pushed a commit to update and add comments to explain all these things. Take a look.

[codecov-commenter]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[mtrippled]

LGTM 👍