Page MenuHomePhabricator

add sigchecks limiter to CheckInputs
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
T704: sigChecks implementation
Commits
rABC6564b850d4b3: add sigchecks limiter to CheckInputs
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Sun, Feb 2, 13:37
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Feb 2, 13:37
deadalnix requested changes to this revision.Sun, Feb 2, 23:00
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
src/validation.cpp
1194 ↗(On Diff #15941)
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 ↗(On Diff #15941)

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 ↗(On Diff #15941)

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

src/validation.h
473 ↗(On Diff #15941)

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 ↗(On Diff #15941)

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 ↗(On Diff #15941)

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

This revision now requires changes to proceed.Sun, Feb 2, 23:00
markblundeberg added inline comments.Mon, Feb 3, 04:13
src/validation.cpp
1219 ↗(On Diff #15941)

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 ↗(On Diff #15941)

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 ↗(On Diff #15941)

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

update, various changes

deadalnix accepted this revision.Tue, Feb 4, 16:12
deadalnix added inline comments.
src/validation.h
480 ↗(On Diff #15941)

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.Tue, Feb 4, 16:12
markblundeberg added inline comments.Wed, Feb 5, 13:56
src/validation.h
480 ↗(On Diff #15941)

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.