T3060
Refactor to parse alias txs per the latest spec.
Details
Details
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- alias-server-to-new-spec
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23766 Build 47142: Build Diff alias-server-tests Build 47141: arc lint + arc unit
Event Timeline
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (alias-server-tests) failed.
Tail of the build log:
Some issues need review, and may require choosing a different dependency. Run `npm audit` for details. > alias-server@1.0.0 test > mocha --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/alias-server-junit.xml --reporter-options testsuitesTitle=Alias Server Unit Tests --reporter-options rootSuiteTitle=Alias Server Connected successfully to MongoDB server Initialized serverState on app startup Configured connection to database ecashAliases Connected successfully to MongoDB server Configured connection to database ecashAliases Connected successfully to MongoDB server Initialized serverState on app startup Configured connection to database ecashAliases Connected successfully to MongoDB server Initialized serverState on app startup Configured connection to database ecashAliases Connected successfully to MongoDB server Initialized serverState on app startup Configured connection to database ecashAliases Connected successfully to MongoDB server Initialized serverState on app startup Configured connection to database ecashAliases Connected successfully to MongoDB server Initialized serverState on app startup Configured connection to database ecashAliases Connected successfully to MongoDB server Configured connection to database ecashAliases ----------------------|---------|----------|---------|---------|------------------------------------ File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ----------------------|---------|----------|---------|---------|------------------------------------ All files | 78.24 | 78.94 | 82.92 | 78.19 | alias-server | 100 | 100 | 100 | 100 | config.js | 100 | 100 | 100 | 100 | secrets.sample.js | 100 | 100 | 100 | 100 | alias-server/src | 78.12 | 78.94 | 82.92 | 78.07 | alias.js | 89.36 | 76.66 | 100 | 89.01 | ...5,93-97,122-126,142,188,243-246 chronik.js | 64.17 | 83.33 | 71.42 | 65.07 | 19,44,72,113-118,151-196 chronikWsHandler.js | 70.37 | 20 | 66.66 | 70.37 | 23,78-94 db.js | 70.83 | 90 | 83.33 | 70.83 | ...109-110,122-123,130-140,154-158 events.js | 88 | 93.75 | 100 | 87.75 | 29-34,90-95,111,183 main.js | 55.17 | 62.5 | 50 | 55.17 | 40,58-80 rpc.js | 93.75 | 83.33 | 100 | 93.75 | 36 telegram.js | 80 | 100 | 75 | 80 | 20 utils.js | 100 | 100 | 100 | 100 | ----------------------|---------|----------|---------|---------|------------------------------------ ##teamcity[blockOpened name='Code Coverage Summary'] ##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='277'] ##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='354'] ##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='75'] ##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='95'] ##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='34'] ##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='41'] ##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='269'] ##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='344'] ##teamcity[blockClosed name='Code Coverage Summary'] Build alias-server-tests failed with exit code 12
| apps/alias-server/scripts/generateMocks.js | ||
|---|---|---|
| 1 | Note: This file was not updated in line with other refactor changes we made to the app, as it was also not used to generate new mocks and was overlooked. These updates were required in this diff as I had to generate new mocks to use the new spec | |
Comment Actions
Let's update the spec in its own diff
| apps/alias-server/src/alias.js | ||
|---|---|---|
| 80 ↗ | (On Diff #40389) | This variable is useless, just put the condition in the if() |
| 98 ↗ | (On Diff #40389) | You should consider a consume(nbytes) method that returns the n bytes and move the cursor. Like: if (consume(stack, 2) != '00') {
// For now, only version 0 alias registration txs are valid
continue;
} |
| 104 ↗ | (On Diff #40389) | This intermediate variable is not useful |
| 108 ↗ | (On Diff #40389) | AFAICT that's not compliant with the spec |
| 117 ↗ | (On Diff #40389) | dito |
| 143 ↗ | (On Diff #40389) | it's type, not version |
| 150 ↗ | (On Diff #40389) | How do you deal with other push types ? |
| 210 ↗ | (On Diff #40389) | What's the point of using an array if the size can only be 1 ? |
| apps/alias-server/test/mocks/aliasMocks.js | ||
| 1 ↗ | (On Diff #40389) | Is this file generated ? If so please add @generated somewhere |
| doc/standards/ecash-alias.md | ||
| 48 ↗ | (On Diff #40389) | So it can be pushed with OP_PUSHDATA4 ? |
Comment Actions
Let's update the spec in its own diff
OK -- made some amendments here just so we can see it agrees with the code as it is in this diff. When the diff is good to go, I will remove the spec changes and put them into their own diff.
| apps/alias-server/src/alias.js | ||
|---|---|---|
| 150 ↗ | (On Diff #40389) | I'm not familiar with every possible push type here. It's easier to control for this by modifying the spec to only support the push type that uses the least data. In this case, no one would ever need to push with 4c or in some other way, so why not constrain available push type to the most efficient in the spec? Supporting 4c is simple enough, but I think I'm still overlooking other options. |
| 210 ↗ | (On Diff #40389) | This approach was selected to handle the edge case of a tx with multiple OP_RETURNS, each a nominally valid alias registration. If I don't find this specific edge case, then such a tx could arbitrarily call the OP_RETURN output with the lowest index a valid alias registration. We could support this in the spec. However, I think it's better to not support, because
An alt approach would be to say in the spec, "alias registration txs must be at the OP_RETURN field with the lowest output index", and do away with this array. |
| doc/standards/ecash-alias.md | ||
| 48 ↗ | (On Diff #40389) | Updated spec to be less ambiguous here. In this case...it could yes, but then the address would be invalid. |
| doc/standards/ecash-alias.md | ||
|---|---|---|
| 48 ↗ | (On Diff #40389) | I don't think this is the right approach. This whole ambiguousity of the spec push format is a hint that whatever you add to this spec, it's a good time to implement a proper method for parsing a script data push. Once you have this method that works for all the different flavors of push, then you can start restricting to some method that makes sense depending on what you're pushing. What you have here is premature optimization and you can be sure that it will break your code when the spec evolves. |
Comment Actions
Let's split this into logical pieces. You will need:
- A consume() function => should be its own diff. It might also be useful outside of the alias server in some other lib, not sure which projects fits best here.
- A feature-complete push parsing function => should be its own diff, same remark about the project to put that in. In any case, not in utils.js or any similar meaningless file, it's a script parsing function.
- Update the spec so we can discuss on the new restrictions
- Update this diff. This should be easy to review as only the useful top-level logic should remain, as opposed to the diff currently doing a ton of unrelated things.
Look at your code, especially your last changes: it should have been obvious that the new functions needed to be split: they are self-contained with their dedicated tests, so they can be reviewed independently.
| apps/alias-server/src/utils.js | ||
|---|---|---|
| 48 ↗ | (On Diff #40443) | You can add this and unit tests in a dedicated diff so it is not hidden by all the other changes |
| 57 ↗ | (On Diff #40443) | That's your consume function. Looks like you want a set of methods in some script.js file. |
| 61 ↗ | (On Diff #40443) | 6a *IS* OP_RETURN, it's not a push |
| 98 ↗ | (On Diff #40443) | Don't do that. This should be a method to parse a push, not something specialized like so. You don't even throw for OP_PUSHDATA1 ! Your method needs to give you the useful data but not interpret them, that not its job. You want to know if the script starting at current position is a push, if yes how it was pushed, how many bytes are pushed, and the direct value if this is a direct push (like OP_4). After you're done you have a push parsing method that would work for all kind of use cases (and maybe it doesn't belong to the alias server ?) which you can call for all the push inside your OP_RETURN. |
| 121 ↗ | (On Diff #40443) | that's 2 bytes |
| 136 ↗ | (On Diff #40443) | In the event of PUSHDATA_4, you might be copying 4GB of data. That's an obvious OOM vector. |