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(simulator): Patch BaseTransaction.resolve() to prevent updating timestamp #641

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

msbrogli
Copy link
Member

@msbrogli msbrogli commented Jun 1, 2023

Acceptance criteria

  1. Patch BaseTransaction.resolve() to prevent updating timestamp.

Notice that BaseTransaction.resolve() calls BaseTransaction.start_mining(), which updates the timestamp using time.time() if mining takes more than two seconds. This might be causing tests to randomly fail.

The reasons for tests to randomly fail is that Transaction.start_mining() is using time.time() to get the timestamp (instead of reactor.seconds()). As the reactor time is not related to the current timestamp, the transaction timestamp can be too much in the future raising a InvalidNewTransaction exception.

For more details, check out this part of the code: https://github.com/HathorNetwork/hathor-core/blob/master/hathor/manager.py#L976-L982

@msbrogli msbrogli requested a review from glevco June 1, 2023 02:36
@msbrogli msbrogli self-assigned this Jun 1, 2023
@msbrogli msbrogli requested a review from jansegre as a code owner June 1, 2023 02:36
@msbrogli msbrogli force-pushed the fix/simulator-patch-resolve branch 2 times, most recently from 09d8bf0 to 0d70490 Compare June 1, 2023 03:32
glevco
glevco previously approved these changes Jun 1, 2023
Copy link
Contributor

@glevco glevco left a comment

Choose a reason for hiding this comment

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

Just curious, why would the timestamp update make tests fail?

hathor/simulator/simulator.py Show resolved Hide resolved
@msbrogli
Copy link
Member Author

msbrogli commented Jun 1, 2023

Just curious, why would the timestamp update make tests fail?

Because Transaction.start_mining() is using time.time() to get the timestamp (instead of reactor.seconds()). As the reactor time is not related to the current timestamp, the transaction timestamp can be too much in the future raising a InvalidNewTransaction exception.

Check out this part of the code: https://github.com/HathorNetwork/hathor-core/blob/master/hathor/manager.py#L976-L982

@msbrogli msbrogli force-pushed the fix/simulator-patch-resolve branch from 0d70490 to 96e8b0f Compare June 1, 2023 16:12
Copy link
Member

@jansegre jansegre left a comment

Choose a reason for hiding this comment

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

Do we still need the timestamp update? Couldn't we just remove it? Or default it to not doing an update and requiring a "timestamp source", like a reactor to do it?

Mostly I'd prefer to avoid doing patches.

@msbrogli msbrogli force-pushed the fix/simulator-patch-resolve branch from 96e8b0f to 0cf0e1d Compare June 1, 2023 18:43
@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #641 (0cf0e1d) into master (84a7516) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
- Coverage   84.06%   84.05%   -0.01%     
==========================================
  Files         245      245              
  Lines       20270    20277       +7     
  Branches     2758     2758              
==========================================
+ Hits        17039    17043       +4     
- Misses       2635     2636       +1     
- Partials      596      598       +2     
Impacted Files Coverage Δ
hathor/simulator/simulator.py 94.61% <100.00%> (+0.23%) ⬆️

... and 2 files with indirect coverage changes

@msbrogli msbrogli merged commit 0cf0e1d into master Jun 1, 2023
@msbrogli msbrogli deleted the fix/simulator-patch-resolve branch June 1, 2023 21:24
@jansegre jansegre mentioned this pull request Jul 12, 2023
2 tasks
This was referenced Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants