Page MenuHomePhabricator

add sigchecks limiter to CheckInputs
ClosedPublic

Authored by markblundeberg on Feb 2 2020, 13:37.

Details

Summary

Unused so far, this will be used to implement the block
level limit on sigchecks.

(A second one could be added the same way later to limit both per-
transaction and per-block at the same time, not worrying about that
for now.)

Test Plan

ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
sigchecks_limit
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9226
Build 16397: Default Diff Build & Tests
Build 16396: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Feb 2 2020, 23:00
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/validation.cpp
1194
if (!VerifyScript(...)) {
    return false;
}
if (pLimitSigChecks && !pLimitSigChecks->check()) {
    error = ScriptError::UNKNOWN;
    return false;
}
return true;

Don't try to carry state around when you don't need state.

1203

There should probably be an error code for this regardless. I think it is fine to assign the sigops limit reached opcode that exist for standard scripts.

1219

Is there any reason to have nSigChecksOut with that design? It seems redundant with the limiter.

src/validation.h
473

Just use int64_t . We don't support any machine for which int_fast64_t is needed and I'd rather have thing always the same on all architectures regardless of perf in the consensus layer anyways. If an architecture doesn't do 64 bits atomic well, then fuck them.

480

If you care about the result, then relaxed is not what you want. I would strongly suggest you just leave the default unless you have a benchmark showing this is winning something, because getting this wrong is both easy and hard to debug.

484

Relaxed is not appropriate here. Relaxed load rarely ever make sense.

This revision now requires changes to proceed.Feb 2 2020, 23:00
src/validation.cpp
1219

Perhaps we can get rid of it later on if it ends up being really redundant, but for now I want the limiting (optional, mutated variable with unspecified initial count; only signals good/bad) and per-tx accounting (required for cache & pure-output variable; any value) to be kept as totally distinct concepts.

src/validation.h
480

I do only need relaxed here because I merely want an atomic RMW operation on this counter, without synchronizing, i.e., without enforcing memory ordering on other loads/stores. (the meaning of the counter is not representing something about the state of memory) And as far as I understand, whether one uses the result or not is orthogonal to the question of memory ordering.

That said, aside from possible caller inlining optimizations it's no difference on x86, only changes things on ARMv8. https://godbolt.org/z/nmBv9T ... and it seems anyway the compilers don't even take advantage of the possible optimizations.

Anyway, I'm happy to leave it as seq_cst for the paranoia reason.

484

Likewise this just needs to be a simple load without a data race -- nothing is being synchronized.

deadalnix added inline comments.
src/validation.h
480

If there is no ordering, then there is no guarantee you ever see the counter going bellow the value you are checking for because decrements can appear to be done in different orders on different threads, none of them being the last one in its own world view.

If you want to understand some of the details of this, then I'll refer you to Herb's atomic weapon talk, especially the part about smart pointer - and how almost everybody in the audience get it wrong.

But also take a step back. The fact that we are having this discussion show that there is a cost to this, and that absolutely no benefit has been demonstrated with a benchmark. This is just the wrong tradeof. The reality is that you could indeed get away with acq_rel consistency rather than sequential, but even that is the wrong tradeof, because there are literally zero measurable upside at this time.

This revision is now accepted and ready to land.Feb 4 2020, 16:12
src/validation.h
480

I've seen those Herb Sutter talks, they're great. :-) Yes with smart pointers there is a need to synchronize other memory, my point is that this is not anything like a smart pointer. There is no other memory location to worry about and all we need is 1) atomicity of the RMW, and 2) modification order consistency of this one location.

I.e., we only need that the update is not torn and that there is an objective ordering to this one memory location (and other memory may be totally desynchronized, doesn't matter). That's exactly what relaxed ordering gives us, and from what I understand this kind of thing is the classic use case for it.

https://en.cppreference.com/w/cpp/atomic/memory_order

All modifications to any particular atomic variable occur in a total order that is specific to this one atomic variable.

In other words: if you have a 0-initialized variable and fetch_add(1) in three threads, it is guaranteed that one thread sees 0 and one thread sees 1 and one thread sees 2; and the final value they all see (after eventually synchronizing later on) is 3. All of this is not influenced by memory ordering options, and how could it be, since this program only has one memory location.