Page MenuHomePhabricator

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

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

Details

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

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bytesofman added inline comments.
chronik/bitcoinsuite-slp/src/empp/parse.rs
214 ↗(On Diff #43689)

Why is this last Ok(()) removed?

Much better

chronik/bitcoinsuite-slp/src/empp/parse.rs
214 ↗(On Diff #43689)

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.

This revision is now accepted and ready to land.Dec 20 2023, 20:01
chronik/bitcoinsuite-slp/src/empp/parse.rs
214 ↗(On Diff #43689)

I removed the return type of Result<(), ParseError>, so we can't have the Ok(()) here anymore.