-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stage transactions not included in swapped chain #775
Conversation
Codecov Report
@@ Coverage Diff @@
## master #775 +/- ##
==========================================
- Coverage 86.43% 86.42% -0.01%
==========================================
Files 226 226
Lines 19427 19496 +69
==========================================
+ Hits 16791 16850 +59
- Misses 1431 1434 +3
- Partials 1205 1212 +7
|
ce7e8ae
to
f6a2e2b
Compare
CHANGES.md
Outdated
@@ -188,6 +188,8 @@ To be released. | |||
[[#759]] | |||
- Fixed a bug where `BlockChain<T>` had rendered and evaluated actions in | |||
the genesis block during forking. [[#763]] | |||
- Fixed a bug where transactions which were not propagated sufficiently, | |||
could not be included in a block when reorg happened. [[#775]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this explanation does not sufficiently scope the situation, e.g., it is vague where's the borderline between sufficient and insufficient propagation. How about this?
Fixed a
Swam<T>
's bug that someTransaction<T>
s had become excluded from miningBlock<T>
s after reorg from α to β where aTransaction<T>
was once included by aBlock<T>
(α) and not included by an otherBlock<T>
(β) for the sameIndex
due to the latency gap between nodes.
Libplanet/Blockchain/BlockChain.cs
Outdated
{ | ||
if (t.Equals(o)) | ||
if (this[index].Equals(other[index])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using Block<T>.PreviousHash
instead of random access?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to use index
, not Block<T>.PreviousHash
because I didn't have any idea for how to traverse, including the genesis block. Do you have some ideas for it?
2f7133c
to
c483e0d
Compare
c483e0d
to
19429ac
Compare
19429ac
to
21ba7f4
Compare
Libplanet.Tests/Net/SwarmTest.cs
Outdated
Assert.Equal((Text)dumbItem, minerA.BlockChain.GetState(_fx1.Address1)); | ||
Assert.Equal((Text)dumbItem, minerB.BlockChain.GetState(_fx1.Address2)); | ||
|
||
Log.Debug("Reorg occurred."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this time the reorg is not happened yet, right?
Log.Debug("Reorg occurred."); | |
Log.Debug("Reorg occurrs."); |
|
||
Log.Debug("Reorg occurred."); | ||
minerB.BroadcastBlock(blockC); | ||
await minerA.BlockAppended.WaitAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… or it might be better to write the log message at this time.
await minerA.BlockAppended.WaitAsync(); | |
await minerA.BlockAppended.WaitAsync(); | |
Log.Debug("Reorg occurred."); |
Co-Authored-By: Hong Minhee <hong.minhee@gmail.com> Co-Authored-By: Ko Chanhyuck <lime_bell@naver.com>
21ba7f4
to
ace2177
Compare
It fixes a bug where transactions included in unrendered blocks, could not be included in a block when reorg happened. It's based on network problem which they were not propagated sufficiently, so IMO it will be better to see this PR while looking message propagation after #767 PR was merged.