Skip to content
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

Fix 2 race conditions with proposer and transaction pool #2352

Merged
merged 7 commits into from May 3, 2019

Conversation

Projects
None yet
3 participants
@enolan
Copy link
Contributor

commented Apr 30, 2019

These were causing the "Invalid user command! Error was: Nonce in account 1
different from nonce in transaction 0" errors in fake_hash:full-test.

In proposer.ml we were getting the current best tip to propose off of it, then
waiting for an IVar, then getting the transaction pool. If the best tip changed
while we were waiting, we would see transactions that were valid against the new
best tip and potentially not the one we basing our proposal on. I changed it to
get the transaction pool at the same time as the best tip, before waiting.

We were also waiting for the proposed transition to be accepted into the
transition frontier before starting to propose again. The second race condition
is when our proposed transition has been accepted and we start to propose again
before the transaction pool is updated to remove now-invalid transactions. I
fixed by changing it to update the diffs (and consequently the transaction
pool) before calling Transition_registry.notify, which wakes up the waiting
proposer. This way the transaction pool is guaranteed to be updated in time.

Tested with unit tests and by running the integration test in question five times. It didn't show the error message.

Checklist:

  • Tests were added for the new behavior
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them:

@enolan enolan requested a review from nholland94 Apr 30, 2019

Fix 2 race conditions with proposer and transaction pool
These were causing the "Invalid user command! Error was: Nonce in account 1
different from nonce in transaction 0" errors in fake_hash:full-test.

In proposer.ml we were getting the current best tip to propose off of it, then
waiting for an IVar, then getting the transaction pool. If the best tip changed
while we were waiting, we would see transactions that were valid against the new
best tip and potentially not the one we basing our proposal on. I changed it to
get the transaction pool at the same time as the best tip, before waiting.

We were also waiting for the proposed transition to be accepted into the
transition frontier before starting to propose again. The second race condition
is when our proposed transition has been accepted and we start to propose again
before the transaction pool is updated to remove now-invalid transactions. I
fixed by changing it to update the diffs (and consequently the transaction
pool) before calling Transition_registry.notify, which wakes up the waiting
proposer. This way the transaction pool is guaranteed to be updated in time.

@enolan enolan force-pushed the fix/proposer-txpool-race branch from b1ec292 to 2517f9c Apr 30, 2019

mergify-bot and others added some commits Apr 30, 2019

@mergify mergify bot merged commit 6d63856 into master May 3, 2019

20 of 22 checks passed

ci/circleci: test--test_postake_catchup Your tests failed on CircleCI
Details
ci/circleci: test--test_postake_holy_grail Your tests failed on CircleCI
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Rule: automatically merge approved PRs with the ready-to-merge label (merge) The pull request has been merged automatically
Details
Summary 1 rule matches
Details
ci/circleci: build-artifacts--testnet_postake Your tests passed on CircleCI!
Details
ci/circleci: build-artifacts--testnet_postake_many_proposers Your tests passed on CircleCI!
Details
ci/circleci: build-artifacts--testnet_postake_snarkless_fake_hash Your tests passed on CircleCI!
Details
ci/circleci: build-wallet Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test--fake_hash Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_bootstrap Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_txns Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_txns Your tests passed on CircleCI!
Details
ci/circleci: test-unit--dev Your tests passed on CircleCI!
Details
ci/circleci: test-unit--test_postake_snarkless Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details

@mergify mergify bot deleted the fix/proposer-txpool-race branch May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.