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.)
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- T704: sigChecks implementation
- Commits
- rSTAGING6564b850d4b3: add sigchecks limiter to CheckInputs
rABC6564b850d4b3: add sigchecks limiter to CheckInputs
ninja check
Diff Detail
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
| 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. | |
| 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. | |
| 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. | |
| 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
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. | |