Considering the miner as CK Pool just made the logic unnecessarily complicated
Details
Details
- Reviewers
bytesofman Fabien - Group Reviewers
Restricted Project
cd apps/ecash-herald
npm install
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- herald-iceberg
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 23926 Build 47462: Build Diff ecash-herald-tests Build 47461: arc lint + arc unit
Event Timeline
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (ecash-herald-tests) failed.
Tail of the build log:
/work/apps/ecash-herald /work/abc-ci-builds/ecash-herald-tests npm WARN deprecated request-promise@4.2.6: request-promise has been deprecated because it extends the now deprecated request package, see https://github.com/request/request/issues/3142 npm WARN deprecated uuid@3.4.0: Please upgrade to version 7 or higher. Older versions may use Math.random() in certain circumstances, which is known to be problematic. See https://v8.dev/blog/math-random for details. npm WARN deprecated har-validator@5.1.5: this library is no longer supported npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142 added 436 packages, and audited 437 packages in 3s 86 packages are looking for funding run `npm fund` for details 4 moderate severity vulnerabilities Some issues need review, and may require choosing a different dependency. Run `npm audit` for details. > ecash-herald@1.0.0 test > mocha --reporter mocha-junit-reporter --reporter-options mochaFile=test_results/ecash-herald-junit.xml --reporter-options testsuitesTitle=Ecash Herald Unit Tests --reporter-options rootSuiteTitle=Ecash Herald ------------------------|---------|----------|---------|---------|-------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ------------------------|---------|----------|---------|---------|-------------------- All files | 96.64 | 86.7 | 96.42 | 96.51 | ecash-herald | 100 | 100 | 100 | 100 | config.js | 100 | 100 | 100 | 100 | ecash-herald/constants | 100 | 100 | 100 | 100 | miners.js | 100 | 100 | 100 | 100 | op_return.js | 100 | 100 | 100 | 100 | ecash-herald/src | 96.61 | 86.7 | 96.42 | 96.49 | chronik.js | 100 | 100 | 100 | 100 | chronikWsHandler.js | 93.75 | 100 | 66.66 | 93.75 | 19 events.js | 83.33 | 80 | 100 | 83.33 | 31,52-56,82 main.js | 81.81 | 80 | 100 | 81.81 | 43-49 parse.js | 97.88 | 85.27 | 100 | 97.79 | 68,113,300,339-342 telegram.js | 95 | 62.5 | 100 | 94.73 | 130-135 utils.js | 100 | 97.43 | 100 | 100 | 111 ------------------------|---------|----------|---------|---------|-------------------- ##teamcity[blockOpened name='Code Coverage Summary'] ##teamcity[buildStatisticValue key='CodeCoverageAbsBCovered' value='403'] ##teamcity[buildStatisticValue key='CodeCoverageAbsBTotal' value='417'] ##teamcity[buildStatisticValue key='CodeCoverageAbsRCovered' value='163'] ##teamcity[buildStatisticValue key='CodeCoverageAbsRTotal' value='188'] ##teamcity[buildStatisticValue key='CodeCoverageAbsMCovered' value='27'] ##teamcity[buildStatisticValue key='CodeCoverageAbsMTotal' value='28'] ##teamcity[buildStatisticValue key='CodeCoverageAbsLCovered' value='388'] ##teamcity[buildStatisticValue key='CodeCoverageAbsLTotal' value='402'] ##teamcity[blockClosed name='Code Coverage Summary'] Build ecash-herald-tests failed with exit code 1
Comment Actions
Need to update test/fixtures/miners.js so that line 105 is IceBerg instead of CK Pool -- since with this change, this is how we would like to label it.
Comment Actions
This is strictly worse than the current implementation.
Before this patch you will detect any CK Pool miner and specifically the only solo miner currently on the chain.
After this patch you will no longer detect any CK Pool miner and assume it's that solo miner.
If anything, simplifying can be achieved by removing the special case so we can print "CKPool, mined by IceBerg" the same way we detect it for via btc (with no special case).
Comment Actions
The problem is that there isn't actually a "CK Pool" mining pool on eCash. As far as I can tell, the "CK pool" message in the coinbase is just a result of a solo miner using this software: https://bitbucket.org/ckolivas/ckpool/src/master/
Comment Actions
If you don't want CKPool to appear, then just don't display it. You can Look at the "mined by" portion and extract the name from here.
Hardcoding Iceberg for all CKPool coinbases means that if someone else starts doing the same, solo mining using the CKPool software, then we won't notice.
Comment Actions
The problem is that there isn't actually a "CK Pool" mining pool on eCash. As far as I can tell, the "CK pool" message in the coinbase is just a result of a solo miner using this software
with the app as it is (in the monorepo, not in prod) -- in this case it does just display "IceBerg"
What's nice about keeping the 'CK Pool' hex fragment is that, if another miner starts using CK Pool software and does not have IceBerg's payoutOutputScript, the app will recognize this is a CK Pool block. Then we can notice that, confirm it isn't Iceberg, and add this miner to the list of knownMiners, with its own name if the miner happens to provide one in their coinbase.