Page MenuHomePhabricator

[WIP] track per-transaction SigChecks using a TransactionMetricsAccumulator
AbandonedPublic

Authored by markblundeberg on Jan 20 2020, 10:29.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T704: sigChecks implementation
Summary

If it is desired to enforce a per-transaction limit on sigchecks at
consensus layer then this needs to be supported by the parallel script
checking system.

Depends on D4940

Test Plan

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sigchecks_tx_accumulator
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9072
Build 16106: Default Diff Build & Tests
Build 16105: arc lint + arc unit

Event Timeline

markblundeberg retitled this revision from track per-transaction SigChecks using a TransactionMetricsAccumulator to [WIP] track per-transaction SigChecks using a TransactionMetricsAccumulator.

is wip

The general approach look good. Obviously, Without changing ScriptExecutionMetrics, it's hard to ensure your invariants hold, so you may want to unpod that guy as well.

src/validation.cpp
1192

You are checking against what you promise the world to do, not what you actual do.

1198

This will always add, which isn't what you want. You want something like:

int txSigCheck = nSigChecks.load();
while(txSigCHeck <= MAX_TX_SIGCHECKS_COUNT) {
    int newTxSigCheck = txSigCheck + metrics.nSigChecks;
    if (nSigChecks.compare_exchange_weak(txSigCheck, newTxSigCheck) {
        return newTxSigCheck <= MAX_TX_SIGCHECKS_COUNT;
    }
}

return false;
src/validation.h
536

I'm not convinced this is the best place, especially since it could be opaque in this header.

538

If this the responsibility of the class to handle its innards, then why this? You'll note that this is simply communicating infos to the outside, because there are actually nothing enforcing that it is used in the inside. This does seems to be achieving anything.

deadalnix requested changes to this revision.Jan 20 2020, 14:16