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.

Sign up for GitHub

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

Jump to bottom

Time-invariant event log #2382

Closed
wants to merge 4 commits into from
Closed

Time-invariant event log #2382

wants to merge 4 commits into from

Conversation

pierluca
Copy link
Contributor

@pierluca pierluca commented Sep 28, 2020 •
edited
Loading

This pull requests deals with #2376 and adds a few documentation fixes.
It makes the event log contract time-invariant by referring exclusively to the current block timestamp rather than time.Now()

I'm still looking into the unit-tests.

🙅‍ Friendly checklist:

  • 0. Code comments are added (or updated) when/where needed and explain the WHY of the code.
  • 1. Design choices, user documentation and any additional doc are added (or updated) in READMEs.
  • 2. Any new behaviour is tested and small units of code that can be are unit tested.
  • 3. Code comments are added on tests to explain what they do.
  • 4. Errors are systematically wrapped with a meaningful message using xerrors.Errorf and the %v verb.
  • 5. Hard limit of 80 chars is always respected.
  • 6. Changes are backward compatible.
  • 7. Indentation level does not exceed 5, although 4 is already suspicious.
  • 8. Functions, files, and packages are kept to a manageable size and decomposed into smaller units if needed.
  • 9. There are no magic values.

@ineiti
Copy link
Member

ineiti commented Sep 28, 2020

Draft-comments: I started doing 2 separate PRs for separating:

  • comments changes that don't change the code
  • actual implementation of what I'm doing
    Of course I do this only once I'm done with the PR itself...

@pierluca pierluca changed the title [WIP] Time-invariant event log Time-invariant event log Sep 30, 2020
@pierluca pierluca marked this pull request as ready for review September 30, 2020 14:37
@pierluca
Copy link
Contributor Author

I'm waiting for Travis to work, based on : #2383 and #2367

ineiti
ineiti requested changes Sep 30, 2020
View reviewed changes
Copy link
Member

@ineiti ineiti left a comment

Choose a reason for hiding this comment

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

Are there tests for this change?

eventlog/service.go
// Because a # of blocks limit is too fragile when using fast blocks for
// tests.)
func decodeAndCheckEvent(rst byzcoin.ReadOnlyStateTrie, eventBuf []byte) (*Event, error) {
// Check the timestamp of the event: it should never be in the future beyond the current block,
Copy link
Member

Choose a reason for hiding this comment

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

you accept 4 * config.BlockInterval, which is fine, but doesn't match your comment here.

eventlog/service.go
// eventDelayTolerance indicates how much time after the event timestamp
// we are stilling willing to accept an event as genuine and contemporaneous,
// and hence as valid. Expressed in [ns]
const eventDelayTolerance = time.Duration(-60 * 1e9)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const eventDelayTolerance = time.Duration(-60 * 1e9)
const eventDelayTolerance = time.Minute

And reverse the comment to say before, as well as changing the code.

@ineiti
Copy link
Member

ineiti commented Oct 5, 2020

You can rebase to master, the updates to the testing framework should make it work more reliable now. And if new tests fail, it'll be easier to make them pass...

@ineiti
Copy link
Member

ineiti commented Oct 5, 2020

In fact the latest travis test failed because of an error in coveralls...

@ineiti
Copy link
Member

ineiti commented Oct 19, 2020

@pierluca bumping this, as the byzcoin.BCTest is now in master and produces quite stable tests.

@pierluca
Copy link
Contributor Author

pierluca commented Oct 19, 2020 •
edited
Loading

Thanks for the information @ineiti.
I'm committed to seeing this through, but it's not my # 1 priority right now.
I'll come back to this as soon as I can find some spare time.

@ineiti
Copy link
Member

ineiti commented Mar 10, 2021

Is this something you think working on, or should it be closed? You might want to delegate it ;)

@pierluca
Copy link
Contributor Author

pierluca commented Apr 6, 2021

Considering Cothority is reaching C4DT support's end-of-life in April 2022 and that this feature is not currently used by any production system, I'm closing this PR. I'd be happy to pick it up again later should it become relevant / a priority.

@pierluca pierluca closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ineiti ineiti ineiti requested changes

Assignees

@pierluca pierluca

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pierluca @ineiti