T3060
Use the consume function to build a feature complete push-parsing function for OP_RETURN.
OOM vector of large pushes is not directly handled. An error will be thrown though if pushdata exceeds the length of the provided string.
Details
Details
- Reviewers
Fabien - Group Reviewers
Restricted Project - Commits
- rABC17997244f09f: [alias-server] push parsing for op return
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| apps/alias-server/constants/op_return.js | ||
|---|---|---|
| 6 ↗ | (On Diff #40523) | deprecated |
| 36 ↗ | (On Diff #40523) | You're missing OP_1NEGATE and OP_RESERVED, even if this is not needed for your use case you'd better support them as well for the generic push parser. |
| 42 ↗ | (On Diff #40523) | OK, good to have this explained |
| 125 ↗ | (On Diff #40523) | If you're defining this, you should probably:
|
| apps/alias-server/src/script.js | ||
| 66 ↗ | (On Diff #40523) | nit: layout |
| 72 ↗ | (On Diff #40523) | consumeNextPush is a bit better imo, no blocker |
| 74 ↗ | (On Diff #40523) | Why do you need that JSON thing here, but you don't need when reassigning the stack in case of error ? |
| 80 ↗ | (On Diff #40523) | This doesn't belong to this function. You want a push: if this is not a push, just raise an error. |
| 104 ↗ | (On Diff #40523) | Looking at the code below, I assume that consume can throw. This means that your strategy to reassign the stack in case of error doesn't work. The simple solution here is to revert the logic: consume on the copy, and if everything is fine then assign it to the input stack |
| 110 ↗ | (On Diff #40523) | You should write methods for reading/writing 16/32 LE/BE bits as an integer, this is a common situation that you're likely to encounter in the future. Can be done after this diff |
| 122 ↗ | (On Diff #40523) | That layout is super weird |
Comment Actions
Responding to feedback
| apps/alias-server/src/script.js | ||
|---|---|---|
| 74 ↗ | (On Diff #40523) | This is a way to clone the stack object, so that when stack is modified by consume, initialStack will not be modified. When reassigning, we are taking the modified stack object and resetting its remainingHex key to this preserved version from initialStack That said...probably more straightforward to just save that key as a string, rather than go through this confusing javascript clone hack. |
| 110 ↗ | (On Diff #40523) | ok added a task for this |
| apps/alias-server/src/script.js | ||
|---|---|---|
| 74 ↗ | (On Diff #40523) | Update: better to keep the clone after changing the logic about preserving stack in case of an error per comment below |
| apps/alias-server/src/script.js | ||
|---|---|---|
| 66 ↗ | (On Diff #40532) | |
| 67 ↗ | (On Diff #40532) | |
| 124 ↗ | (On Diff #40532) | This should be obvious from the function doc, see suggested edit above |
| apps/alias-server/test/scriptTests.js | ||
| 175 ↗ | (On Diff #40532) | Just don't create the OP_RETURN in the first place.
None of this require the relatively complicated stack of pushs from a real alias, which will be tested anyway at the upper layer logic. You can dramatically simplify the tests with a bunch of simple and well crafted test vectors. |
Comment Actions
You're missing a test for the case the pushdata length doesn't match the operator, like my previous example 4d42 (aka PUSHDATA2 42)
Comment Actions
parsing 0x4d42 will fail and the stack will be modified.
In this case, the failure will depend on what is next in the stack.
e.g.
0x4d4274657374 i.e. 0x4d42 + ascii encoded "test" => parser will see 4d and get 4274, flip this to 7442, parseInt('7442', 16) = 29762 --> so it will fail because the provided string is much shorter than this.
In theory the app could still a case where the push operator is used "wrong" but there is still a result...in this case, well, that is what the script says.
Added test cases