Ledger: Correct off by one logic in txTail.recent trim loop #4615
Conversation
|
|
AlgoAxel
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4615 +/- ##
==========================================
- Coverage 54.27% 54.22% -0.05%
==========================================
Files 402 402
Lines 51809 51814 +5
==========================================
- Hits 28119 28097 -22
- Misses 21330 21351 +21
- Partials 2360 2366 +6 ☔ View full report in Codecov by Sentry. |
|
Don't we need 1001 now to support `block Timestamp` opcode?
…On Fri, Sep 30, 2022, 3:33 PM AlgoAxel ***@***.***> wrote:
Summary What
This change modifies the loop condition for trimming txTail's recent list
during comittedUpTo calls.
Why
It was pointed out that the equality check used was avoiding deletion of
one entry, leaving 1001 entries in the list. The expectation is that only
1000 entries would remain after being trimmed.
How
I duplicated the loop which handles deletion, and added a consensus flag
to control which version is used. If consensus is on future, it will use
the modified logic, and trim down to 1000 entries. Else, the existing
behavior is retained.
Presumably, once this feature is adopted via consensus, we can remove the
branched logic and consensus flag.
Test Plan
✅ make generate
✅ make sanity
✅ make test
✅ make integration (still running, but all look good so far)
*Unit Test*: I added an explicit length check to an existing unit test
for txTail, which confirms that the length is correct to the consensus
version which is selected.
Additionally: I logged the length of txtail.recent before and after the
affected loop, and also logged any deletion actions taken.
Before
before: 1002","name":"","time":"2022-09-28T16:06:16.435378-04:00"}
deleted 24428225","name":"","time":"2022-09-28T16:06:16.435465-04:00"}
after: 1001","name":"","time":"2022-09-28T16:06:16.435492-04:00"}
After
before: 1001","name":"","time":"2022-09-28T16:08:55.860060-04:00"}
deleted 24428270","name":"","time":"2022-09-28T16:08:55.860285-04:00"}
after: 1000","name":"","time":"2022-09-28T16:08:55.860342-04:00"}
------------------------------
You can view, comment on, or merge this pull request online at:
#4615
Commit Summary
- 0a039f2
<0a039f2>
Correct off by one logic in txTail.recent trim loop
File Changes
(3 files <https://github.com/algorand/go-algorand/pull/4615/files>)
- *M* config/consensus.go
<https://github.com/algorand/go-algorand/pull/4615/files#diff-a75ef7294adbd7ae646c49ae2a518253936983c0a16594b84f20f15403767a2f>
(9)
- *M* ledger/txtail.go
<https://github.com/algorand/go-algorand/pull/4615/files#diff-dd62ed3016cd25cfae4381b11ee1a24f2a3bf50f0bb47721207617a7228be6a0>
(17)
- *M* ledger/txtail_test.go
<https://github.com/algorand/go-algorand/pull/4615/files#diff-a7b47f183164def4c780bba70e25c19d36b6b99f6d94f5e5d8a09967b1be678e>
(7)
Patch Links:
- https://github.com/algorand/go-algorand/pull/4615.patch
- https://github.com/algorand/go-algorand/pull/4615.diff
—
Reply to this email directly, view it on GitHub
<#4615>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADL7T3LBXEZVBVEEFRADCTWA46AHANCNFSM6AAAAAAQ2BQ634>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
There should be no impact to opcodes, as |
This sounds right. I thought you were discussing the txtail as a whole. For example, |
| // TXTailRecentsFix modifies the logic of the txtail's comittedUpTo function to address an off-by-one-error | ||
| // txtail previously trimmed up to (but not including) round `r + maxlife`. Doing this left 1001 transaction objects after trimming. | ||
| // the intention is to have only 1000 items after trim, so the logic is modified to trim *including* round `r + maxlife` | ||
| TXTailRecentsFix bool |
There was a problem hiding this comment.
I don't think this change will have an impact on the consensus. This is an extra element which is sitting and taking up space that nobody depends on. Therefore, TXTailRecentFix is not needed.
@algorandskiy do you agree?
Otherwise, will be nice to describe how this is impacting the consensus.
There was a problem hiding this comment.
The idea is that if there are different behaviors of TX Tail, transactions would be distributed by some nodes, and discarded by other nodes, since they wouldn't agree on if the lease was still valid on that 1000/1001 threshold. It was originally a discussion with Pavel that prompted this feature flag, but I'll leave it to him for the deeper answer.
There was a problem hiding this comment.
We need a test to demonstrate this disagreement. I don't think we have one now. At least not a unit test.
There was a problem hiding this comment.
That could be part of why @algorandskiy had suggested the Protocol flag in the first place -- by enforcing the recents length at the protocol level, there is no concern that peers will disagree, so no test would be needed on that (right?).
Summary
What
This change modifies the loop condition for trimming txTail's
recentlist duringcomittedUpTocalls.Why
It was pointed out that the equality check used was avoiding deletion of one entry, leaving 1001 entries in the list. The expectation is that only 1000 entries would remain after being trimmed.
How
I duplicated the loop which handles deletion, and added a consensus flag to control which version is used. If consensus is on future, it will use the modified logic, and trim down to 1000 entries. Else, the existing behavior is retained.
Presumably, once this feature is adopted via consensus, we can remove the branched logic and consensus flag.
Test Plan
✅
make generate✅
make sanity✅
make test✅
make integration(still running, but all look good so far)Unit Test: I added an explicit length check to an existing unit test for txTail, which confirms that the length is correct to the consensus version which is selected.
Additionally: I logged the length of
txtail.recentbefore and after the affected loop, and also logged any deletion actions taken.Before
After