Page MenuHomePhabricator

Impose a minimum transaction size of 100 bytes after the Nov, 15 2018 HF
ClosedPublic

Authored by deadalnix on Jul 29 2018, 18:28.

Details

Summary

This ensure we are not vulnerable to the leaf node weakness in bitcoin's merkle tree design anymore and give us room to evolve the protocol in the future (or not).

For more information about the weakness, please read https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/ ( http://archive.is/xA9mW ).

Test Plan

Added unit test that ensure transaction size is properly checked after activation.
Added an integration test to check for activation.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Mengerian requested changes to this revision.Jul 29 2018, 21:31
Mengerian added a subscriber: Mengerian.
Mengerian added inline comments.
src/validation.cpp
3620 ↗(On Diff #4468)

Un-capitalize "i" in "nMedianTimePast" ?

test/functional/abc-magnetic-anomaly-activation.py
2 ↗(On Diff #4468)

Should probably keep the Bitcoin Core copyright bloat from abc-transaction-ordering.py

test/functional/abc-transaction-ordering.py
2 ↗(On Diff #4468)

Same here, add copyright bloat

This revision now requires changes to proceed.Jul 29 2018, 21:31

Fix typo

test/functional/abc-magnetic-anomaly-activation.py
2 ↗(On Diff #4468)

They did not write that test to begin with.

test/functional/abc-transaction-ordering.py
2 ↗(On Diff #4468)

Well they did not write that test either.

Mengerian accepted this revision.
This revision is now accepted and ready to land.Aug 4 2018, 17:28
jasonbcox requested changes to this revision.Aug 5 2018, 16:10
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/miner.cpp
206 ↗(On Diff #4491)

Is - 1 necessary here? If coinbaseSize is N bytes short, why add N+1 bytes?

src/test/transaction_tests.cpp
763 ↗(On Diff #4491)

Either rename this test to txsize_activation_test or add cases for boundary conditions. The integration tests cover the MIN_TX_SIZE and MIN_TX_SIZE - 1 boundary, so I think simply renaming is adequate for now.

src/validation.cpp
3718–3721 ↗(On Diff #4491)

Doesn't this spam the logs being in this for-loop? Either remove or move it to a more appropriate place.

test/functional/abc-magnetic-anomaly-activation.py
113 ↗(On Diff #4491)

Looks like a copy-paste error? This function is redefined a few lines below this one.

This revision now requires changes to proceed.Aug 5 2018, 16:10
src/miner.cpp
206 ↗(On Diff #4491)

One byte for the push.

test/functional/abc-magnetic-anomaly-activation.py
113 ↗(On Diff #4491)

It's weird that it works at all.

  • Remove spammy logs
  • Fix double definition of get_tests
  • Rename txsize_test into txsize_activation_test
jasonbcox added inline comments.
src/miner.cpp
206 ↗(On Diff #4491)

Got it.

This revision is now accepted and ready to land.Aug 5 2018, 16:24
This revision was automatically updated to reflect the committed changes.

I'm very sorry for this late review.

Since this change is in response to a very specific vulnerability, it should impose only what is required to address the vulnerability. The vulnerability applies only to transactions whose length is exactly 64 bytes, so if a size restriction is used to address the vulnerability, only that exact size should be restricted.

Globally forbidding transactions < 100 bytes is likely to affect more future use-cases unnecessarily, and create a situation where this change is referenced as a bug that introduced the problem. The time to avoid this situation is now.

IMHO A better overall solution, given the limited potential for exploit of the vulnerability, would be to design a new merkle proof format and migrate to it over time.