Page MenuHomePhabricator

Implement canonical transaction ordering.

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



This ensure that transaction are ordered by txid going forward.

Depends on D1528, D1527

Test Plan

Added integration tests.

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
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.

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?

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

Yes, but this is out of scope.

3807 ↗(On Diff #4177)


jasonbcox requested changes to this revision.Jul 5 2018, 21:59
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
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.
3807 ↗(On Diff #4177)

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

schancel added inline comments.
240 ↗(On Diff #4177)

Why are these exactly equal?

254 ↗(On Diff #4177)


259 ↗(On Diff #4177)

Should these be exactly equal?

426 ↗(On Diff #4177)


schancel requested changes to this revision.Jul 6 2018, 17:13
schancel added inline comments.
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

3770 ↗(On Diff #4177)

Yes, but why do you trust me on this ?

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.