Details
- Reviewers
schancel jasonbcox - Group Reviewers
Restricted Project - Commits
- rSTAGINGee51761f7792: Implement canonical transaction ordering.
rABCee51761f7792: Implement canonical transaction ordering.
Added integration tests.
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- canonicalorder
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 2856 Build 3816: Bitcoin ABC Buildbot (legacy) Build 3815: arc lint + arc unit
Event Timeline
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:
Does this make sense to you? |
| src/validation.cpp | ||
| 3807 ↗ | (On Diff #4177) | Is the prevTx check here just an optimization? |
| 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. |
| src/validation.cpp | ||
|---|---|---|
| 3807 ↗ | (On Diff #4177) | coinbase must be at position zero. This is enforced and tested already. |
| 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. |
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.