consensus lib work: add status codes to the script interpreter and remove logging #5212

theuni · 2014-11-05T01:14:37Z

Cleaned up version of #5188

Attempt to codify the possible error statuses associated with script
validation. The second commit (move-only) moves the other flags into the newly
created script/types.h as part of the upcoming external interface.

Logging has also been removed in order to drop the dependency on util.h. It can
be re-added to bitcoind as-needed. This makes script verification finally free
of application state and boost!

sipa · 2014-11-05T06:38:32Z

@theuni I was actually thinking not to expose the script verification flags externally, as they are implementation and policy specific, but instead have the interface define its own flags, which get translated into internal ones.

theuni · 2014-11-05T06:42:42Z

I'll defer to you on that. I can nuke the top commit and save that for a separate discussion.

sipa · 2014-11-05T06:53:05Z

src/script/types.h

These two are also under BIP62 (they're also used for standardness checks outside of BIP62, but that shouldn't matter for a consensus lib).

gmaxwell · 2014-11-05T10:44:17Z

With respect to the concern that callers might use the error code to decide pass/fail instead of the actual return. Why not set the error value to the unknown error at entry, so long as an error pointer is provided at all?

theuni · 2014-11-06T07:15:23Z

Updated for @sipa's and @gmaxwell's comments.

laanwj · 2014-11-07T09:37:57Z

src/script/interpreter.cpp

Nit: can you add the error messages that you remove here as comment? ie

- if (vchPubKey.size() < 33) + if (vchPubKey.size() < 33) // Non-canonical public key: too short - return error("Non-canonical public key: too short"); + return false;

We do lose some troubleshooting information here, as the error mechanism provides no way to signal warnings/details of script interpretation. But I don't see an alternative either. Maybe in the future it would be useful to add a way to get a detailed trace of execution.

I started to add a bitmask for more detailed info in the case of ERR_EVAL_VALSE or ERR_VERIFY_*, but that ended up looking a little over-engineered and error-prone. If you think it's worth it, i can re-add that.

That's not necessary for this pull

But please do add the comments as suggested, so that the cases are documented and if later on we have a way to return details execution status, we can return it.

laanwj · 2014-11-07T09:54:53Z

Looks good to me, utACK (will test)

laanwj · 2014-11-07T09:57:04Z

src/script/script_error.cpp

Currently this function is unused! Shouldn't we call this function where we use EvalScript/VerifyScript, and log the message?

@laanwj I didn't want to hook it up in each place, since it could be potentially much noisier than the old logging. To be close to on-par with current behavior, we could check for something like SCRIPT_ERR_SIG_DER || SCRIPT_ERR_SIG_HASHTYPE || SCRIPT_ERR_SIG_HIGH_S. Those are pretty arbitrary though.

Yes, it's pretty arbitrary. But it's also strange to have a function that is called from nowhere, not even from the unit tests. But I'm fine with doing that in a later pull, and regarding this as preparation.

@laanwj Yes, I'd prefer to address the logging in a later pull, as I assumed that would lead to some bikeshedding. Though I agree that it wouldn't make sense to push this without using it anywhere, so I've updated the tests to check the values.

theuni · 2014-11-11T23:42:45Z

@laanwj Sorry, I missed your comment (about comments) the first time around. re-added those and rebased on master.

TheBlueMatt · 2014-11-13T02:34:56Z

src/script/interpreter.cpp

Isnt this unknown opcode?

yes, thanks for catching that!

TheBlueMatt · 2014-11-13T03:00:11Z

ACK commithash 4eefa373ae4866a1ad25cea3615270f58693e6a7 (+/- the incorrect error message on unknown opcode): http://bitcoin.ninja/TheBlueMatt-5212.txt

laanwj · 2014-11-14T11:17:44Z

src/script/interpreter.cpp

Sorry to nit, but indentation is wrong here (and in some other places where comments are added)

sipa · 2014-11-14T11:20:32Z

@theuni to get back to an earlier comment I made: if #5247 would be merged, non-standard public key encoding do result in script failure immediately, so IsCompressedOrUncompressedPublicKey could set a status code etc, like you did earlier.

laanwj · 2014-11-14T11:24:20Z

theuni · 2014-11-14T21:14:59Z

@sipa Ok, thanks. If/when that happens, I'll follow-up with the more detailed values.

theuni · 2014-11-14T21:22:53Z

Squashed. This is code-identical to the last push except for the comments white-space nit fix.

…ve logging Attempt to codify the possible error statuses associated with script validation. script/types.h has been created with the expectation that it will be part of the public lib interface. The other flag enums will be moved here in a future commit. Logging has also been removed in order to drop the dependency on core.h. It can be re-added to bitcoind as-needed. This makes script verification finally free of application state and boost!

sipa · 2014-11-17T15:40:38Z

utACK

219a147 script: check ScriptError values in script tests (Cory Fields) ab9edbd script: create sane error return codes for script validation and remove logging (Cory Fields)

theuni mentioned this pull request Nov 5, 2014

RFC: Remove logging from script interpreter #5188

Closed

sipa reviewed Nov 5, 2014
View reviewed changes

theuni force-pushed the reducedeps12 branch from 4e6dd2f to f9787fe Compare November 6, 2014 05:43

theuni mentioned this pull request Nov 7, 2014

RFC: libbitcoinconsensus #5235

Merged

laanwj reviewed Nov 7, 2014
View reviewed changes

theuni force-pushed the reducedeps12 branch from f9787fe to 4eefa37 Compare November 11, 2014 23:41

TheBlueMatt reviewed Nov 13, 2014
View reviewed changes

theuni force-pushed the reducedeps12 branch from 61e3d35 to ddadf2d Compare November 13, 2014 19:30

laanwj reviewed Nov 14, 2014
View reviewed changes

src/script/interpreter.cpp Outdated

Copy link

Member

laanwj Nov 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to nit, but indentation is wrong here (and in some other places where comments are added)

theuni force-pushed the reducedeps12 branch from ddadf2d to 219a147 Compare November 14, 2014 21:21

theuni added 2 commits November 14, 2014 16:25

script: check ScriptError values in script tests

219a147

TheBlueMatt mentioned this pull request Nov 14, 2014

[Feature Request] Spoiler support in comments (eg for PGP signature "compression") github/markup#411

Closed

laanwj merged commit 219a147 into bitcoin:master Nov 17, 2014

laanwj added a commit that referenced this pull request Nov 17, 2014

Merge pull request #5212

8adf457

219a147 script: check ScriptError values in script tests (Cory Fields) ab9edbd script: create sane error return codes for script validation and remove logging (Cory Fields)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

consensus lib work: add status codes to the script interpreter and remove logging #5212

consensus lib work: add status codes to the script interpreter and remove logging #5212

Uh oh!

Conversation

theuni commented Nov 5, 2014

Uh oh!

sipa commented Nov 5, 2014

Uh oh!

theuni commented Nov 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Nov 5, 2014

Uh oh!

theuni commented Nov 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Nov 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 14, 2014

Uh oh!

laanwj commented Nov 14, 2014

Uh oh!

theuni commented Nov 14, 2014

Uh oh!

theuni commented Nov 14, 2014

Uh oh!

sipa commented Nov 17, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants