Page MenuHomePhabricator

[consensus rule] limit sigchecks in a block after phonon upgrade
ClosedPublic

Authored by markblundeberg on Feb 6 2020, 13:32.

Details

Summary

Depends on D5127 D5029 D5220

Test Plan
ninja check-extended
ninja check-upgrade-activated-extended

Rerun ninja check-all with cmake .. -DBUILD_BITCOIN_WALLET=OFF.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

TODO: make a test that probes virtualsize priorities using sigchecks

deadalnix added inline comments.
src/miner.cpp
259 ↗(On Diff #16074)

You can cache that result instead of nExcessiveBlockSize

src/miner.h
140 ↗(On Diff #16074)

You may want to store the sigcheck limit instead of the excessive block size so you don't need to recompute it every time.

156 ↗(On Diff #16074)

Please find a better name for this. For instance, fUseSigCheck .

src/validation.cpp
738 ↗(On Diff #16074)

This is really trashy, but we'll have to live with this for a few month. Please keep in mind to clean this up.

1684 ↗(On Diff #16074)

Is that important that both flags are different?

1891 ↗(On Diff #16074)

I'm not sure how CheckInputsLimiter came to be named that way, but it obviously suck. It doesn't say what it is limiting (so the name don't give any meaningful info as to what this *IS*), but it does say what it is passed to, which is an abstraction failure.

Imagine if you buy a car to go to work, and so you decide to call it a workgoer instead of a car. Well then it gets very confusing when everybody starts to use the workgoer to go to the gym, in holidays, or wherever.

src/validation.cpp
738 ↗(On Diff #16074)

Yep, trashy is the right word. :D

1684 ↗(On Diff #16074)

I prefer it that way... they are both going to be removed anyway so those bits will be freed up soon enough.

This diff definitely does need to turn on this flag though.

1891 ↗(On Diff #16074)

Perhaps. We can rename it later.

address comments; simplify using D5220

test/functional/abc-block-sigchecks-activation.py
38 ↗(On Diff #16172)

(fixme)

deadalnix requested changes to this revision.Feb 9 2020, 03:18

Some details can be improved, but it is good overall.

src/miner.cpp
87 ↗(On Diff #16172)

Why are you creating a local variable that you then assign to the field rather than assign to the field directly? Unless I'm missing something, this is completely redundant.

src/txmempool.h
82 ↗(On Diff #16172)

We usually use double star.

test/functional/abc-block-sigchecks-activation.py
154 ↗(On Diff #16172)

Just do node = self.nodes[0] instead of showing off :)

262 ↗(On Diff #16172)

dito

This revision now requires changes to proceed.Feb 9 2020, 03:18
src/miner.cpp
87 ↗(On Diff #16172)

Yes, just to discriminate the two concepts: 1) calculating the consensus limit, and 2) setting the maximum we are willing to generate. Analogous to the nExcessiveBlockSize / nMaxGeneratedBlockSize distinction.

For example we might consider seting nMaxGeneratedBlockSigChecks = min(nMaxBlockSigChecks , nMaxGeneratedBlockSize * density_limit ) ... i.e., let's say I'm a miner generating only 2 MB blocks, I might decide I don't want to have a density worse than 50 bytes/sigcheck (so max 50000 sigchecks) even though the consensus limit is 32 MB blocks and >200000 sigchecks.

test/functional/abc-block-sigchecks-activation.py
262 ↗(On Diff #16172)

Technically it's better this way since if the RHS length is not exactly 1, it will throw. :-D

update per comments; still todo: virtualsize test

markblundeberg retitled this revision from [WIP] [consensus rule] limit sigchecks in a block after phonon upgrade to [consensus rule] limit sigchecks in a block after phonon upgrade.Feb 12 2020, 01:01

I think I am going to leave off the virtualsize tests to a separate Diff, to keep this focussed on consensus behaviour. (It's not like we're introducing any new virtualsize behaviours, just changing what count is used)

This revision is now accepted and ready to land.Feb 12 2020, 15:04

update func test to work with nowallet build

jasonbcox added a subscriber: jasonbcox.

In the interest of time, I'm not going to block this diff, but this really needs some release notes attached. Feel free to incorporate the SigCheck note that I started in D5179