Page MenuHomePhabricator

Remove topological ordering constraint from block starting Nov, 15 2018
ClosedPublic

Authored by deadalnix on Jun 7 2018, 08:51.

Details

Summary

As per title.

Depends on D1484

Test Plan

Added an integration test checking for the proper activation of this feature.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
txorder
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2715
Build 3542: Bitcoin ABC Buildbot (legacy)
Build 3541: arc lint + arc unit

Event Timeline

Mengerian requested changes to this revision.Jun 8 2018, 05:28
Mengerian added a subscriber: Mengerian.
Mengerian added inline comments.
test/functional/abc-transaction-ordering.py
12

This comment is confusing.

Seems like it should be changed to mention transaction ordering?

This revision now requires changes to proceed.Jun 8 2018, 05:28

Fix test description comment

jasonbcox requested changes to this revision.Jun 17 2018, 01:59
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/abc-transaction-ordering.py
50 ↗(On Diff #4100)

I see this used in a few tests, but don't see it used anywhere. What does it do?

90 ↗(On Diff #4100)

engough -> enough

227 ↗(On Diff #4100)

acivate -> activate

240 ↗(On Diff #4100)

Actiavte -> Activate

249 ↗(On Diff #4100)

Isn't block 4444 already out of order? 4444 doesn't look like it was made to be in-order, so I think this line is redundant.

This revision now requires changes to proceed.Jun 17 2018, 01:59
test/functional/abc-transaction-ordering.py
50 ↗(On Diff #4100)

It's not used, removing.

249 ↗(On Diff #4100)

Block 4444 is before the feature activates and is rejected.

jasonbcox added inline comments.
test/functional/abc-transaction-ordering.py
249 ↗(On Diff #4100)

We cleared this up offline. For posterity: the block number *is not* height. The height is the order they're generated. I think the numbers can be replaced with variable names instead, but I'll take care of that improvement later.

This revision is now accepted and ready to land.Jun 19 2018, 14:34
This revision was automatically updated to reflect the committed changes.