Feat/timestamp block #2897
Feat/timestamp block #2897
Conversation
types/tipset_test.go
Outdated
| @@ -153,7 +154,12 @@ func TestTipSet(t *testing.T) { | |||
| // String shouldn't really need testing, but some existing code uses the string as a | |||
| // datastore key and depends on the format exactly. | |||
| assert.Equal(t, "{ "+b1.Cid().String()+" }", RequireNewTipSet(t, b1).String()) | |||
| assert.Equal(t, "{ "+b1.Cid().String()+" "+b2.Cid().String()+" "+b3.Cid().String()+" }", | |||
|
|
|||
| // DONOTMETGE the below assertion needed to be fixed after adding a timestamp to block. | |||
There was a problem hiding this comment.
@anorth could you confirm my suspicion here, blame shows you as the author. Does the order of these matter?
There was a problem hiding this comment.
Yes, order matters (from SortedCidSet)
There was a problem hiding this comment.
You can robustify the test by actually using a sortedcidset to order them.
frrist
force-pushed
the
feat/timestamp-block
branch
from
fdbf911 to
1f59eee
Compare
Codecov Report
@@ Coverage Diff @@
## master #2897 +/- ##
========================================
Coverage ? 43%
========================================
Files ? 208
Lines ? 12370
Branches ? 0
========================================
Hits ? 5349
Misses ? 6211
Partials ? 810
|
plumbing/clock/testing.go
Outdated
| import ( | ||
| "time" | ||
|
|
||
| "github.com/benbjohnson/clock" |
There was a problem hiding this comment.
I noticed from looking that the issues that this isn't actively maintained and has some open issues about races (they might actually be addressed but issues are open). Probably fine but something to keep in mind if we start seeing races in tests using it.
There was a problem hiding this comment.
Looks like the de-racing PR got closed down without merging and the author recommended this project: https://github.com/jonboulle/clockwork.
There was a problem hiding this comment.
Ohh nice find! I remember looking at that issue but didn't see the recommendation, will address and bubble up the changes to the validation branch.
There was a problem hiding this comment.
As mentioned in #2894, a clock should be fully independent of the blocktime configuration value, and will be used more widely. Unlike the configuration, I don't think the clock should be presented through plumbing (at least not yet). Just directly inject into the components that need it at construction (in node).
In chat I had questioned the use of an external dependency for this, but having inspected it a bit more this seems appropriate.
plumbing/clock/testing.go
Outdated
| // NewMockBlockClock returns a new MockBlockClock with the provided blockTime. | ||
| // Calls to MockBlockClock.EpochSeconds() will return 0 until a time is set using | ||
| // the method SetClock(). | ||
| func NewMockBlockClock(blkTime time.Duration) *MockBlockClock { |
There was a problem hiding this comment.
I suggest taking the mock time value in this constructor, saving most users a call to SetClock
frrist
changed the base branch from
feat/miner-worker-clock
to
feat/blocktime-porcelain
frrist
force-pushed
the
feat/timestamp-block
branch
from
1f59eee to
1cc19ce
Compare
frrist
force-pushed
the
feat/blocktime-porcelain
branch
from
8ac5b9f to
d14b976
Compare
frrist
force-pushed
the
feat/timestamp-block
branch
2 times, most recently
from
b2caa77 to
3a5fe58
Compare
frrist
force-pushed
the
feat/blocktime-porcelain
branch
2 times, most recently
from
f676111 to
03af606
Compare
frrist
force-pushed
the
feat/timestamp-block
branch
2 times, most recently
from
ac0d78c to
acbcc58
Compare
frrist
force-pushed
the
feat/blocktime-porcelain
branch
from
03af606 to
def89fb
Compare
frrist
force-pushed
the
feat/timestamp-block
branch
from
acbcc58 to
da4344d
Compare
What
This PR adds the timestamp field to block. This PR must be merged after #2914