Page MenuHomePhabricator

[alias-server] Parse aliases according to latest spec
AbandonedPublic

Authored by bytesofman on May 17 2023, 23:56.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Summary

T3060

Refactor to parse alias txs per the latest spec.

Test Plan

npm test

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 Diffalias-server-tests
Build 47141: arc lint + arc unit

Event Timeline

Need more mocks and tests

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

Refactor unit tests to work with mocks updated to spec

Undo manual mods to generateMocks

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

Remove debug logging, update spec per this implementation

Fabien requested changes to this revision.May 24 2023, 17:34
Fabien added a subscriber: Fabien.

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 ?

This revision now requires changes to proceed.May 24 2023, 17:34
bytesofman marked 7 inline comments as done.

Responding to feedback. Add 'consume' function, remove impractical variables.

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

  • Will probably never happen
  • Significant complication to the app (instead of checking for false or {object}, need to check parseAliasTx for [array] as well, for every single time it is called
  • Ambiguity in handling edge case of multiple OP_RETURNs, each registering an alias, but total fee paid does not cover all the registrations. In this case, which one is valid?

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.

Fabien requested changes to this revision.May 25 2023, 12:22
Fabien added inline comments.
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.

This revision now requires changes to proceed.May 25 2023, 12:22

Improving OP_RETURN processing function

Fabien requested changes to this revision.May 26 2023, 05:35

Let's split this into logical pieces. You will need:

  1. 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.
  2. 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.
  3. Update the spec so we can discuss on the new restrictions
  4. 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.

This revision now requires changes to proceed.May 26 2023, 05:35