[Chronik] Change `empp::parse` to return `None` for ignorable results
ClosedPublic
Actions

Authored by tobias_ruck on Dec 20 2023, 16:15.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

Summary

Currently, ParseError::should_ignore tells the call site whether to ignore the failed parsing attempt.

However, it is more intuitive to return Ok(None) in this case.

Also, we change the semantics slightly to make the parsing error a bit more precise:

If the script has invalidly encoded opcodes (e.g. OP_PUSHDATA1 with a missing length), but doesn't start with OP_RETURN OP_RESERVED, we return None
If the script does start with OP_RETURN OP_RESERVED, we return the DataError associated with the failed parse from the opcode.

This means users get an error for the bad script (which would fail policy rules anyway) a bit earlier, if it does look like they tried to use eMPP.

Test Plan

cargo test -p bitcoinsuite-slp

Repository

rABC Bitcoin ABC

Branch

chronik-empp-parse-none

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 26010
Build 51594: Build Diff	build-chronik · chronik-client-integration-tests
Build 51593: arc lint + arc unit

tobias_ruck requested review of this revision.Dec 20 2023, 16:15

bytesofman added inline comments.

chronik/bitcoinsuite-slp/src/empp/parse.rs
214	Why is this last Ok(()) removed?

Much better

chronik/bitcoinsuite-slp/src/empp/parse.rs
214	The prototype of the function has been changed to no longer return `Result<(), ParseError>`. This is a test function so the returned value is useless.