Skip to content

Add tests for uncle timestamps and refactor timestamp type#1711

Merged
fjl merged 1 commit intoethereum:developfrom
Gustav-Simonsson:timestamp_big_int
Aug 25, 2015
Merged

Add tests for uncle timestamps and refactor timestamp type#1711
fjl merged 1 commit intoethereum:developfrom
Gustav-Simonsson:timestamp_big_int

Conversation

@Gustav-Simonsson
Copy link
Copy Markdown

No description provided.

@robotally
Copy link
Copy Markdown

Vote Count Reviewers
👍 3 @fjl @karalabe @zelig
👎 0

Updated: Tue Aug 25 13:49:16 UTC 2015

@Gustav-Simonsson Gustav-Simonsson force-pushed the timestamp_big_int branch 3 times, most recently from 0659fb1 to df8688f Compare August 24, 2015 14:32
Comment thread core/chain_makers.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2nd arg why not parent.Time().Uint64()?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Some tests do not initialise parent in the first call to this function, and I wanted to avoid changing them just for this patch.

@zelig zelig added this to the 1.1.0 milestone Aug 24, 2015
Comment thread core/block_processor.go
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Although valid, comparing against 1 is a bit odd. Usually big int comparison is done as:

  • < 0 if the first is smaller
  • == 0 if they are equal
  • > 0 if the first one is greater

This scheme makes it a bit more visible as the comparison operator already "symbolizes" the actual result vs. having to interpret -1, or 1. It is used like that throughout the entire standard lib too, so better to stick with the canonical use.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@karalabe I'd wait with this since the comparisons for extra data, difficulty and gas uses the "odd" convention - better we change this everywhere in a follow-up PR so we avoid touching unrelated code in this patch.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok with me then, but it would be nice to update them at a certain point. :)

@zelig
Copy link
Copy Markdown
Contributor

zelig commented Aug 25, 2015

👍 modulo comments

@karalabe
Copy link
Copy Markdown
Member

👎 Until the immutability is fixed.

Also a question. The verify header does not check against negative values. Previously since we unmarshalled into an uint64, this was not an issue. Now however a timestamp big.Int may become negative. We should make sure that such cases are handled correctly.

@fjl
Copy link
Copy Markdown
Contributor

fjl commented Aug 25, 2015

@karalabe RLP cannot contain negative big ints.

Comment thread core/types/block.go
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Gustav-Simonsson please also update copyHeader

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

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.

6 participants