Page MenuHomePhabricator

Implement canonical transaction ordering.
ClosedPublic

Authored by deadalnix on Jul 1 2018, 19:13.

Details

Summary

This ensure that transaction are ordered by txid going forward.

Depends on D1528, D1527

Test Plan

Added integration tests.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
canonicalorder
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2783
Build 3676: Bitcoin ABC Buildbot (legacy)
Build 3675: arc lint + arc unit

Event Timeline

Remove changes included by error

schancel requested changes to this revision.Jul 2 2018, 16:03
schancel added a subscriber: schancel.

Please let me know the answers to my questions.

src/miner.cpp
178

I suppose this is the way it needs to be done in order to have proper activation. In the future I'd like to change this so that:

  1. The mempool is kept sorted and it is just iterated over a new index.
  2. Ensure parent transaction is in the block.
  3. Add transactions based on the fee policy if (2) is satisfied.

Does this make sense to you?

src/validation.cpp
3807

Is the prevTx check here just an optimization?

This revision now requires changes to proceed.Jul 2 2018, 16:03
deadalnix added inline comments.
src/miner.cpp
178

Yes, but this is out of scope.

src/validation.cpp
3807

Yes.

jasonbcox requested changes to this revision.Jul 5 2018, 21:59
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/validation.cpp
3807

Something doesn't seem right with this. Lets say there are 3 txs, C (coinbase), D, E, where the TxId comparison is D < C < E and ordering is [D, C, E]. It will pass this code block when it should be an invalid ordering. Perhaps this function should have an explicit check for coinbase at index 0 before this code block, despite other parts of the code likely enforcing this?

Regardless of the final solution to this, I think this case needs a unit test.

This revision now requires changes to proceed.Jul 5 2018, 21:59
deadalnix added inline comments.
src/validation.cpp
3807

coinbase must be at position zero. This is enforced and tested already.

schancel added inline comments.
test/functional/abc-transaction-ordering.py
240

Why are these exactly equal?

254

11!11!11!!!

259

Should these be exactly equal?

test/functional/test_framework/mininode.py
426

thr

schancel requested changes to this revision.Jul 6 2018, 17:13
schancel added inline comments.
src/validation.cpp
3770

Can we get a comment here as to what "Contextual" means at least, as compared to checkblock? It's not clear to me what should be in one or the other.

Is it that this is based on the context of the block's position in the chain?

3807

This is done in CheckBlock. His point is that if that is changed, this may not fail in some cases edge cases.

This revision now requires changes to proceed.Jul 6 2018, 17:13

Fix typos

src/validation.cpp
3770

Yes, but why do you trust me on this ?

src/validation.cpp
3807

Right, looking at the comment // Check that all transactions are finalized and the error message "Transaction order is invalid (%s < %s)", these are hinting to me that transaction ordering is being validated here. If it's not doing full validation, that needs to be clear and indicate how/why doing partial validation is safe by design.

I guess the change looks fine on its own, but the whole function could do with more commenting on its intent.

This revision is now accepted and ready to land.Jul 11 2018, 19:32

Fix replay protection test

This revision was automatically updated to reflect the committed changes.