Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- T704: sigChecks implementation
- Commits
- rSTAGING276a95b8710e: [consensus rule] limit sigchecks in a block after phonon upgrade
rABC276a95b8710e: [consensus rule] limit sigchecks in a block after phonon upgrade
ninja check-extended ninja check-upgrade-activated-extended
Rerun ninja check-all with cmake .. -DBUILD_BITCOIN_WALLET=OFF.
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- sigchecks_blocklimit
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 9460 Build 16838: Default Diff Build & Tests Build 16837: arc lint + arc unit
Event Timeline
| 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. |
| test/functional/abc-block-sigchecks-activation.py | ||
|---|---|---|
| 38 ↗ | (On Diff #16172) | (fixme) |
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 |
| 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 |
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)
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