Add tests for uncle timestamps and refactor timestamp type #1711
Conversation
Gustav-Simonsson
force-pushed
the
timestamp_big_int
branch
3 times, most recently
from
0659fb1 to
df8688f
Compare
| return &types.Header{ | ||
| Root: state.Root(), | ||
| ParentHash: parent.Hash(), | ||
| Coinbase: parent.Coinbase(), | ||
| Difficulty: CalcDifficulty(time, parent.Time(), parent.Number(), parent.Difficulty()), | ||
| Difficulty: CalcDifficulty(time.Uint64(), new(big.Int).Sub(time, big.NewInt(10)).Uint64(), parent.Number(), parent.Difficulty()), |
There was a problem hiding this comment.
2nd arg why not parent.Time().Uint64()?
There was a problem hiding this comment.
Some tests do not initialise parent in the first call to this function, and I wanted to avoid changing them just for this patch.
Gustav-Simonsson
force-pushed
the
timestamp_big_int
branch
from
df8688f to
a5f4bce
Compare
| if block.Time > uint64(time.Now().Unix()) { | ||
| return BlockFutureErr | ||
| if uncle { | ||
| if block.Time.Cmp(common.MaxBig) == 1 { |
There was a problem hiding this comment.
Although valid, comparing against 1 is a bit odd. Usually big int comparison is done as:
< 0if the first is smaller== 0if they are equal> 0if 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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Ok with me then, but it would be nice to update them at a certain point. :)
|
👍 modulo comments |
|
👎 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. |
Gustav-Simonsson
force-pushed
the
timestamp_big_int
branch
from
a5f4bce to
08c5f10
Compare
|
@karalabe RLP cannot contain negative big ints. |
| @@ -301,13 +301,13 @@ func (b *Block) Number() *big.Int { return new(big.Int).Set(b.header.Number) | |||
| func (b *Block) GasLimit() *big.Int { return new(big.Int).Set(b.header.GasLimit) } | |||
| func (b *Block) GasUsed() *big.Int { return new(big.Int).Set(b.header.GasUsed) } | |||
| func (b *Block) Difficulty() *big.Int { return new(big.Int).Set(b.header.Difficulty) } | |||
| func (b *Block) Time() *big.Int { return new(big.Int).Set(b.header.Time) } | |||
There was a problem hiding this comment.
@Gustav-Simonsson please also update copyHeader
Gustav-Simonsson
force-pushed
the
timestamp_big_int
branch
from
08c5f10 to
7324176
Compare
|
👍 |
1 similar comment
|
👍 |
Add tests for uncle timestamps and refactor timestamp type
Add tests for uncle timestamps and refactor timestamp type (cherry picked from commit abce099)
No description provided.