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 non-deterministic mempool reorg test #531

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

pinheadmz
Copy link
Member

This is the correct approach to fixing this issue, replacing #518 which still left us with non-deterministic outcomes.

I realized the problem is that the test called fakeClaim() TWICE, meaning the stub resolver actually requests cloudflare's DNSSEC chain twice from the actual legacy DNS. The non-determinism is a result of the stub resolver getting a slightly different result the second time, in particular the RRSIG over the TXT RRs in the final zone. Likely there are many redundant resolvers caching cloudflare.com's TXT RRs and the second fakeClaim() call sometimes ended up getting a slightly newer signature, meaning its inception value exceeded the current chain tip time in the test, and failed with bad-claim-time.

Here's the output of the two fakeClaim() proof data from the original test -- you can see the inception and expiration fields are different -- this is always the case when the test fails. Sometimes the test will succeed if it actually got these two signatures in reverse order (the bug is only thrown if the second proof is newer).

ProofData {
  name: 'cloudflare',
  target: 'cloudflare.com.',
  weak: true,
  commitHash: <Buffer 31 c6 30 af 33 54 f0 3f 20 cc 13 8b 93 84 37 2a 2d 04 b6 d1 06 c2 38 33 f8 d1 83 ec 60 c0 57 73>,
  commitHeight: 1,
  inception: 1609966800,
  expiration: 1610123176,
  fee: 984,
  value: 6800503513487,
  version: 0,
  hash: <Buffer f9 e1 b7 dd 35 34 d5 b6 c6 f8 7f e2 0a d8 89 e5 a5 b2 2d b6>
}
ProofData {
  name: 'cloudflare',
  target: 'cloudflare.com.',
  weak: true,
  commitHash: <Buffer 58 89 27 05 1a 20 a3 c1 2a ff a5 cc f0 d7 f2 ce 18 90 e9 71 e0 e5 66 1f c8 46 7d fe f1 35 5b 44>,
  commitHeight: 120,
  inception: 1609992000,
  expiration: 1610123310,
  fee: 984,
  value: 6800503513487,
  version: 0,
  hash: <Buffer 60 1c 88 fc a7 a0 6e 97 62 3b 5d 45 93 13 e4 b4 a5 15 15 05>
}

The new approach is to only request the DNSSEC chain once at the start of the test, and then modify the commitHash and commitHeight fields once the test chain time is already in the valid range.

@coveralls
Copy link

coveralls commented Jan 7, 2021

Pull Request Test Coverage Report for Build 478431252

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.005%) to 59.417%

Files with Coverage Reduction New Missed Lines %
lib/protocol/consensus.js 1 82.79%
Totals Coverage Status
Change from base Build 478034033: -0.005%
Covered Lines: 19430
Relevant Lines: 30472

💛 - Coveralls

@pinheadmz pinheadmz added this to the v2.3.0 milestone Jan 11, 2021
@pinheadmz
Copy link
Member Author

Screen Shot 2021-01-11 at 3 02 13 PM

@rithvikvibhu
Copy link
Member

OS: Manjaro Linux 20.2
Node.js: 14.15.1
hs1qjafwwfv630ldnsveh83lq2zak062r5494zgak3

All tests pass!
image

@pinheadmz
Copy link
Member Author

hs1qjafwwfv630ldnsveh83lq2zak062r5494zgak3

Sent 01ddc94673ef693f7809222060bbae49d74067899a2f06a2c341cd8dc40b2796 thank you!

@pinheadmz pinheadmz merged commit d384f6a into handshake-org:master Jan 12, 2021
Copy link
Contributor

@tuxcanfly tuxcanfly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. Can confirm that it cuts down the time by about half 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants