Page MenuHomePhabricator
Differential D1611

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

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

Details

Reviewers
Mengerian
jasonbcox
Group Reviewers
Restricted Project
Commits
rSTAGINGde3668a2b572: Impose a minimum transaction size of 100 bytes after the Nov, 15 2018 HF
rABCde3668a2b572: Impose a minimum transaction size of 100 bytes after the Nov, 15 2018 HF
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
Branch
mintxsize
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2994
Build 4082: Bitcoin ABC Buildbot (legacy)
Build 4081: arc lint + arc unit

Event Timeline

deadalnix created this revision.Jul 29 2018, 18:28
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 29 2018, 18:28
teamcity added a subscriber: teamcity.Jul 29 2018, 18:30

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Jul 29 2018, 19:09

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2994: Diff 4468.Jul 29 2018, 19:10
Mengerian requested changes to this revision.Jul 29 2018, 21:31
Mengerian added a subscriber: Mengerian.
Mengerian added inline comments.
src/validation.cpp
3620

Un-capitalize "i" in "nMedianTimePast" ?

test/functional/abc-magnetic-anomaly-activation.py
2

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

test/functional/abc-transaction-ordering.py
2

Same here, add copyright bloat

This revision now requires changes to proceed.Jul 29 2018, 21:31
deadalnix updated this revision to Diff 4482.Aug 3 2018, 14:49

Fix typo

test/functional/abc-magnetic-anomaly-activation.py
2

They did not write that test to begin with.

test/functional/abc-transaction-ordering.py
2

Well they did not write that test either.

teamcity added a comment.Aug 3 2018, 14:51

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Aug 3 2018, 15:28

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3005: Diff 4482.Aug 3 2018, 15:28
deadalnix updated this revision to Diff 4491.Aug 4 2018, 13:46

Ping ?

teamcity added a comment.Aug 4 2018, 13:48

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Aug 4 2018, 14:27

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3011: Diff 4491.Aug 4 2018, 14:28
Mengerian accepted this revision as: Mengerian.Aug 4 2018, 17:27
Mengerian accepted this revision.
This revision is now accepted and ready to land.Aug 4 2018, 17:28
deadalnix added a child revision: D1623: Make push only mandatory when magnetic anomaly activates..Aug 5 2018, 15:44
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
deadalnix added inline comments.Aug 5 2018, 16:19
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.

deadalnix updated this revision to Diff 4498.Aug 5 2018, 16:21
  • Remove spammy logs
  • Fix double definition of get_tests
  • Rename txsize_test into txsize_activation_test
teamcity added a comment.Aug 5 2018, 16:23

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox accepted this revision.Aug 5 2018, 16:24
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
teamcity added a comment.Aug 5 2018, 17:03

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3018: Diff 4498.Aug 5 2018, 17:03
Closed by commit rABCde3668a2b572: Impose a minimum transaction size of 100 bytes after the Nov, 15 2018 HF (authored by deadalnix). · Explain WhyAug 5 2018, 19:20
This revision was automatically updated to reflect the committed changes.
dgenr8 added a subscriber: dgenr8.EditedAug 14 2018, 18:58

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.

PiRK mentioned this in D15039: Remove LOCKTIME_MEDIAN_TIME_PAST constant.Jan 3 2024, 01:36

Revision Contents
Changeset List

PathSize
src/
consensus/
2 lines
1 line
16 lines
test/
15 lines
22 lines
3 lines
24 lines
test/
functional/
abc-magnetic-anomaly-activation.py
abc-transaction-ordering.py
123 lines
21 lines
test_framework/
6 lines
4 lines
142 lines

Diff 4468

View Options

src/consensus/consensus.h

Loading...
View Options

src/miner.h

Loading...
View Options

src/miner.cpp

Loading...
View Options

src/test/miner_tests.cpp

Loading...
View Options

src/test/transaction_tests.cpp

Loading...
View Options

src/validation.h

Loading...
View Options

src/validation.cpp

Loading...
View Options

test/functional/abc-magnetic-anomaly-activation.py

Loading...
View Options

test/functional/abc-transaction-ordering.py

Loading...
View Options

test/functional/test_framework/blocktools.py

Loading...
View Options

test/functional/test_framework/cdefs.py

Loading...
View Options

test/functional/timing.json

Loading...