Page MenuHomePhabricator

[herald] Upgrade miner parsing
AbandonedPublic

Authored by bytesofman on May 9 2023, 21:16.

Details

Reviewers
PiRK
Fabien
Group Reviewers
Restricted Project
Summary

T2972

Improve miner parsing.

  • Adds more miners
  • Updates the schema for miners in config.js for better unit testing
  • Implements unit testing

A few steps, but imo made sense to do this in one shot.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
herald-miner-parsing
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23666
Build 46946: Build Diffecash-herald-tests
Build 46945: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.May 10 2023, 12:10
Fabien requested changes to this revision.May 10 2023, 13:32
Fabien added a subscriber: Fabien.
Fabien added inline comments.
apps/ecash-herald/config.js
50 ↗(On Diff #40240)

The test vectors have nothing to do in config.js

This revision now requires changes to proceed.May 10 2023, 13:32
bytesofman added inline comments.
apps/ecash-herald/config.js
50 ↗(On Diff #40240)

This isn't the only thing currently living in config.js that isn't really a config and should require a diff to change. Could move this and also the opReturn constants into their own files.

imo that's beyond the scope of this diff.

I've added a task to improve this organization T3162

I'd like to keep the test vectors in the same place as the miners, so it's clear that adding another miner requires adding associated test blocks in this format.

apps/ecash-herald/src/utils.js
208

maybe this should be in ecashaddrjs but this is the first time i've needed this method. will add it if it comes up again.

apps/ecash-herald/src/parse.js
48 ↗(On Diff #40240)

You should pass this as a parameter. That makes the test easier as you can craft any tx/pattern you want and make sure you have coverage for all the detection methods.

73 ↗(On Diff #40240)

This whole IFP match is totally useless. You're losing more time with this branch than it takes to just fail matching the IFP script.
Just loop until miner is found or you're running out of outpoint.

173 ↗(On Diff #40240)

If that's not going to be reused anywhere it's not worth making it a function. It makes testing less efficient as you don't supply a list of the know miners

apps/ecash-herald/src/utils.js
208 ↗(On Diff #40240)

This is a copy paste of the same function in alias server.
What do you conclude ?

apps/ecash-herald/config.js
50 ↗(On Diff #40240)

This isn't the only thing currently living in config.js that isn't really a config and should require a diff to change. Could move this and also > > the opReturn constants into their own files.

imo that's beyond the scope of this diff.

Agreed, and that should be done first so that this diff can be rebased on top.

I'd like to keep the test vectors in the same place as the miners, so it's clear that adding another miner requires adding associated test
blocks in this format.

That rational makes zero sense. The whole point of unit tests is to make sure the function does what it claims. The unit tests are not supposed to run on every single possible case ! You're testing some happy paths and the edges cases, then the function does always the same job so unless there is a new feature, you don't need more test. You're not testing block parsing on the whole blockchain, and update the test vectors for every new block right ?

apps/ecash-herald/src/utils.js
208

That's wrong and that's my point. It's the second time already, and this is causing code duplication + test duplication. This is not OK.

Fabien requested changes to this revision.May 10 2023, 14:06
This revision now requires changes to proceed.May 10 2023, 14:06
bytesofman added inline comments.
apps/ecash-herald/config.js
50 ↗(On Diff #40240)

Adding another miner could require changes to the getMinerFromCoinbaseTx function.

Need a unit test to prove getMinerFromCoinbaseTx works with an added miner. We may or may not have a block in blocks.js for this new miner; for example I don't think it's worth adding a new block to blocks.js just to cover the lone zpool block we have seen so far. But we do need a unit test to prove that getMinerFromCoinbaseTx correctly recognizes zpool miner.

Will shelf this diff while I work on some of the pre-reqs. What kind of organization would you like to see here?

apps/ecash-herald/src/utils.js
208 ↗(On Diff #40240)

lol. forgot about this.

I did actually write it from the ground up here...again.

D13883

see D13930, this will be rebased onto that