[Config Change] Removes HardReorg config from InboxReader #2542
Conversation
cla-bot
bot
added
the
s
diegoximenes
changed the title
| Require(t, err) | ||
|
|
||
| // Reorg out the user delayed message | ||
| err = tracker.ReorgDelayedTo(1, true) |
There was a problem hiding this comment.
Instead of just removing this test, we should instead have this test insert an alternative delayed message at position 1 which would cause the same reorg. Instead of getting the delayed count below, we could then get the delayed message to ensure it changed.
There was a problem hiding this comment.
I adjusted the test to trigger a reorg by adding an alternative delayed message through AddDelayedMessages as you mentioned, but the strategy that I implemented is a little bit different than the one you described.
diegoximenes
dismissed stale reviews from ganeshvanahalli and gligneul
via
94a07f0
diegoximenes
force-pushed
the
rm_hard_reorg
branch
from
0d584ee
to
94a07f0
Compare
| }, | ||
| }, | ||
| } | ||
| err = tracker.AddDelayedMessages([]*DelayedInboxMessage{userDelayedModified}) |
There was a problem hiding this comment.
I find it weird that, if instead I add an alternative delayed message related to delayedRequestId2, then userMsgBatch, and emptyBatch, will not be deleted.
So maybe I am missing something.
There was a problem hiding this comment.
That is unexpected. Could you push up a failing test case with delayedRequestId2? I can take a look from there
There was a problem hiding this comment.
I pushed a commit that adds TestSequencerReorgFromLastDelayedMsg
|
I think there is an issue in the inbox tracker that your test exposed. I pushed up a commit to this branch to fix it. Let me know what you think |
| @@ -462,6 +462,7 @@ func (t *InboxTracker) AddDelayedMessages(messages []*DelayedInboxMessage) error | |||
| } | |||
| } | |||
|
|
|||
| firstPos := pos | |||
There was a problem hiding this comment.
If I'm right, that will force a reorg of any batch added in any of the messages[] received.
But there is a possibility that some of the messages were already known and have not changed.
A few lines above we have:
if haveLastAcc == messages[len(messages)-1].AfterInboxAcc() {
// We already have these delayed messages
return nil
}
I think that we want to test it for each message and use for firstPos the first one which it isn't true.
There was a problem hiding this comment.
Ah my bad, I read the loop above it and thought it did this but it only happens for snap sync apparently. I'll probably just modify the loop to always happen. Thanks for catching that.
There was a problem hiding this comment.
I've addressed this and updated the test to catch this issue.
Resolves NIT-1349
HardReorg config from InboxReader seems unused so this PR removes it.