Details

Reviewers

deadalnix
Mengerian

Group Reviewers

Restricted Project

Commits

rSTAGING5e82569b10e0: [refactor multisig] make const values up front
rABC5e82569b10e0: [refactor multisig] make const values up front

Summary

Currently during the first phase of multisig, we calculate a variety
of indices and counts that then get mutated later on in
various loops, in a rather hodge-podge fashion.

This Diff changes the values calculated up front to be all const, with better
variable names, and then the mutated variables for each loop are initialized
adjacent to the loops in question. This makes it easier to logically
consider each loop in isolation.

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

rms1

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6784
Build 11615: Bitcoin ABC Buildbot (legacy)
Build 11614: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jul 10 2019, 01:22

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 10 2019, 01:22

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

@deadalnix I know you want to switch it to size_t but I'd like to defer that until after this refactor... I'm not 100% comfortable with the C++ casting rules and I think it would make more sense as a consistent style change over the entire EvalScript function, not just one opcode.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6784: Diff 10178.Jul 10 2019, 01:32

markblundeberg added a child revision: D3626: [refactor multisig] separate nullfail from stack cleanup.Jul 10 2019, 01:38

deadalnix requested changes to this revision.Jul 10 2019, 02:01

deadalnix added inline comments.

src/script/interpreter.cpp
972	Then 1 is the index of nKeysCount and the stack size check ensure that this index is within bounds, so it should use it as well.
983	Presumably, that would be the element immediately after the index of nKeysCount.
989	Making the index size_t as they should be avoid the need for this.
1006	This changes the semantic. Please do it in another diff.
1059	That loop is still horribly screwed up, but at least the screwing up is now limited to the loop.
1077	Extract this in another diff.

This revision now requires changes to proceed.Jul 10 2019, 02:01

markblundeberg added inline comments.Jul 10 2019, 02:12

src/script/interpreter.cpp
989	Some of these indices get used in arithmetic with ints, for example `-idxTopSig - k` below ... it's not immediately apparent to me that it maintains the same behaviour if the indices are now size_t. Again, I'd like to defer the size_t thing to a later diff.
1006	Can you elaborate on what has changed? Do you mean the change from `i` to `idxDummy`? At this point, `i` points to the dummy element, so nothing has changed.

introduce idxKeyCount and restore redundant stack size check

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

@deadalnix I'm trying to find style guides and many like this say that we ought to avoid using unsigned types in this kind of situation, for example: https://google.github.io/styleguide/cppguide.html#Integer_Types

"try to avoid unsigned types (except for representing bitfields or modular arithmetic). Do not use an unsigned type merely to assert that a variable is non-negative."

Can you point to a rationale where I can find out why usage of size_t for this kind of general situation is a Good Thing?

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6786: Diff 10181.Jul 10 2019, 02:26

deadalnix requested changes to this revision.Jul 10 2019, 02:27

deadalnix added inline comments.

src/script/interpreter.cpp
989	You are introducing new variables, make them the right type. This does the same thing. There is no such thing as a signed or an unsigned add for the CPU: https://c9x.me/x86/html/file_module_x86_id_5.html . size_t is the correct type here. Get familiar with 2 complement math if you are not convinced, but I can assure you this is the case. CPU do subtraction by doing via A - B = A + ~B + 1, so it's all additions. The only thing you'll get using an int is to risk the compiler to insert masks in various places to ensure things fit on 32 bits, and the optimizer will assuming that arithmetic operation do not overflow, which you usualy don't want.
1006	ok

This revision now requires changes to proceed.Jul 10 2019, 02:27

markblundeberg added inline comments.Jul 10 2019, 02:40

src/script/interpreter.cpp
989	I know how two's complement arithmetic works, I'm just concerned about type casts since from what I read, an int can in principle be smaller, equal, or larger in size than a size_t in different architectures. I'm not convinced that an int is wrong here, or that a size_t is right.

@deadalnix I've thought over more and I'm not convinced at all about converting to size_t. We can continue to debate but the longer this goes on, the more chance it means that dependent diffs will be delayed, resulting in Schnorr multisig being postponed until May or a later upgrade. That's fine, there's no rush to get these things done, but it seems unfortunate that feature work would be blocked on a nit like this.

deadalnix requested changes to this revision.Jul 10 2019, 14:58

deadalnix added inline comments.

src/script/interpreter.cpp
989	Every single use is either used to compare with the size of a vector or index in a vector. This is 100% a size_t . Doing the wrong thing in the consensus code to match deadline is definitively the wrong tradeof The only way to not get impacted by delay is to start earlier.

This revision now requires changes to proceed.Jul 10 2019, 14:58

guessing at what types are expected

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6852: Diff 10280.Jul 13 2019, 02:34

deadalnix accepted this revision.Jul 13 2019, 10:49

deadalnix added inline comments.

src/script/interpreter.cpp
1006 ↗	(On Diff #10280)	You can remove than line.

This revision is now accepted and ready to land.Jul 13 2019, 10:49

markblundeberg added inline comments.Jul 13 2019, 15:15

src/script/interpreter.cpp
1006 ↗	(On Diff #10280)	👍

I would suggest updating the description, which refers to "ints".

Also, the comment about removing the redundant stack check is no longer valid.

Mengerian added inline comments.Jul 13 2019, 16:28

src/script/interpreter.cpp
986 ↗	(On Diff #10280)	Minor nit: it's missing "is"... You could also change it to reflect the "stack depth" language of the comments for the other indices.

The logic looks good to me.

A few minor comment and commit description nits noted above.

markblundeberg edited the summary of this revision. (Show Details)Jul 13 2019, 19:47

update comments & remove unneeded empty line

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6853: Diff 10283.Jul 13 2019, 23:57

markblundeberg edited the summary of this revision. (Show Details)Jul 14 2019, 00:23

markblundeberg added a child revision: D3673: [refactor multisig] move nulldummy check to front.Jul 14 2019, 00:37

markblundeberg removed a child revision: D3626: [refactor multisig] separate nullfail from stack cleanup.Jul 14 2019, 00:44

Mengerian added inline comments.Jul 14 2019, 05:14

src/script/interpreter.cpp
964 ↗	(On Diff #10283)	Another suggestion: Maybe this comment could be changed to use the names "nSigsCount" and "nKeysCount" rather than "num_of_signatures" and "num_of_pubkeys". This would make it more obvious in relation to the code below.

Mengerian added inline comments.Jul 14 2019, 19:34

src/script/interpreter.cpp
964 ↗	(On Diff #10283)	Actually, since the comment and variable names are pre-existing it could be considered out-of-scope for this Diff. May as well just land it as-is.