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

Resync pending state if needed before adding new transactions #4520

Merged
merged 1 commit into from Sep 21, 2017

Conversation

Projects
None yet
4 participants
@gumb0
Member

gumb0 commented Sep 19, 2017

This should fix the assert happening during sync:
https://github.com/ethereum/cpp-ethereum/blob/develop/libethereum/Block.cpp#L326

It happened when m_working in Client is not up-to-date with the latest modifications to BlockChain and then we tried to import new transactions from TransactionQueue

@@ -659,7 +658,7 @@ void Client::doWork(bool _doWait)
bool isSealed = false;
DEV_READ_GUARDED(x_working)
isSealed = m_working.isSealed();
if (!isSealed && !isSyncing() && !m_remoteWorking && m_syncTransactionQueue.compare_exchange_strong(t, false))

This comment has been minimized.

@gumb0

gumb0 Sep 19, 2017

Member

This will make processing of pending transactions to start closer to sync finish.
(isSyncing() returns false when there are no more blocks to download, but there still may be many blocks to import in BlockQueue)

@gumb0

gumb0 Sep 19, 2017

Member

This will make processing of pending transactions to start closer to sync finish.
(isSyncing() returns false when there are no more blocks to download, but there still may be many blocks to import in BlockQueue)

@gumb0 gumb0 added needs review and removed in progress labels Sep 19, 2017

@gumb0 gumb0 requested a review from chfast Sep 19, 2017

@gumb0 gumb0 added in progress and removed needs review labels Sep 19, 2017

@gumb0

This comment has been minimized.

Show comment
Hide comment
@gumb0

gumb0 Sep 19, 2017

Member

Wait, this leads to deadlock

Member

gumb0 commented Sep 19, 2017

Wait, this leads to deadlock

@gumb0 gumb0 removed the request for review from chfast Sep 19, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 19, 2017

Codecov Report

Merging #4520 into develop will increase coverage by 0.09%.
The diff coverage is 17.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4520      +/-   ##
===========================================
+ Coverage    63.15%   63.25%   +0.09%     
===========================================
  Files          364      364              
  Lines        30413    30423      +10     
  Branches      2750     2752       +2     
===========================================
+ Hits         19208    19243      +35     
+ Misses       11022    10996      -26     
- Partials       183      184       +1
Impacted Files Coverage Δ
libethereum/Client.cpp 26.75% <17.85%> (+1.18%) ⬆️
test/tools/jsontests/StateTests.cpp 91.26% <0%> (+0.26%) ⬆️
libethereum/EthereumHost.cpp 9.89% <0%> (+0.27%) ⬆️
test/unittests/libweb3jsonrpc/Client.cpp 32.61% <0%> (+0.65%) ⬆️
libethereum/BlockQueue.h 71.6% <0%> (+1.23%) ⬆️
libethereum/BlockChainSync.cpp 7.51% <0%> (+1.34%) ⬆️
test/unittests/libethereum/BlockQueue.cpp 51.66% <0%> (+1.47%) ⬆️
libethereum/BlockChainSync.h 18.18% <0%> (+9.09%) ⬆️
libethereum/VerifiedBlock.h 93.75% <0%> (+25%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d58cb3...f5b3ddc. Read the comment docs.

codecov-io commented Sep 19, 2017

Codecov Report

Merging #4520 into develop will increase coverage by 0.09%.
The diff coverage is 17.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4520      +/-   ##
===========================================
+ Coverage    63.15%   63.25%   +0.09%     
===========================================
  Files          364      364              
  Lines        30413    30423      +10     
  Branches      2750     2752       +2     
===========================================
+ Hits         19208    19243      +35     
+ Misses       11022    10996      -26     
- Partials       183      184       +1
Impacted Files Coverage Δ
libethereum/Client.cpp 26.75% <17.85%> (+1.18%) ⬆️
test/tools/jsontests/StateTests.cpp 91.26% <0%> (+0.26%) ⬆️
libethereum/EthereumHost.cpp 9.89% <0%> (+0.27%) ⬆️
test/unittests/libweb3jsonrpc/Client.cpp 32.61% <0%> (+0.65%) ⬆️
libethereum/BlockQueue.h 71.6% <0%> (+1.23%) ⬆️
libethereum/BlockChainSync.cpp 7.51% <0%> (+1.34%) ⬆️
test/unittests/libethereum/BlockQueue.cpp 51.66% <0%> (+1.47%) ⬆️
libethereum/BlockChainSync.h 18.18% <0%> (+9.09%) ⬆️
libethereum/VerifiedBlock.h 93.75% <0%> (+25%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d58cb3...f5b3ddc. Read the comment docs.

@gumb0 gumb0 added needs review and removed in progress labels Sep 19, 2017

@gumb0 gumb0 requested a review from chfast Sep 19, 2017

@chfast

chfast approved these changes Sep 20, 2017

@gumb0 gumb0 merged commit 3536178 into develop Sep 21, 2017

6 of 7 checks passed

codecov/patch 17.85% of diff hit (target 50%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 63.25% (+0.09%) compared to 8d58cb3
Details
codecov/project/app 55.36% (+0.11%) compared to 8d58cb3
Details
codecov/project/tests 76.44% (+0.04%) compared to 8d58cb3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chriseth chriseth removed the needs review label Sep 21, 2017

@gumb0 gumb0 deleted the fix-tq-sync-assert branch Sep 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment