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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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 ↗(On Diff #4177)

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 ↗(On Diff #4177)

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 ↗(On Diff #4177)

Yes, but this is out of scope.

src/validation.cpp
3807 ↗(On Diff #4177)

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 ↗(On Diff #4177)

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 ↗(On Diff #4177)

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

schancel added inline comments.
test/functional/abc-transaction-ordering.py
240 ↗(On Diff #4177)

Why are these exactly equal?

254 ↗(On Diff #4177)

11!11!11!!!

259 ↗(On Diff #4177)

Should these be exactly equal?

test/functional/test_framework/mininode.py
426 ↗(On Diff #4177)

thr

schancel requested changes to this revision.Jul 6 2018, 17:13
schancel added inline comments.
src/validation.cpp
3770 ↗(On Diff #4177)

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 ↗(On Diff #4177)

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 ↗(On Diff #4177)

Yes, but why do you trust me on this ?

src/validation.cpp
3807 ↗(On Diff #4177)

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.