Page MenuHomePhabricator

[CashAddr] replace bitcoincash: prefix with ecash: in the node
Needs ReviewPublic

Authored by majcosta on Dec 17 2020, 20:14.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

Changes node to use ecash: CashAddr prefix instead of bitcoincash: (with ectest: and ecregtest: for testnet and regressiontest, respectively)

Test Plan
cmake .. -GNinja
ninja check-all

Event Timeline

majcosta retitled this revision from [CashAddr] set each network CashAddr prefixes in chainparams.cpp to [CashAddr] replace bitcoincash: prefix with abc:.Dec 17 2020, 20:18
majcosta retitled this revision from [CashAddr] replace bitcoincash: prefix with abc: to [CashAddr] replace bitcoincash: prefix with abc: in the node.
jasonbcox requested changes to this revision.Dec 17 2020, 22:36
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/qt/test/bitcoinaddressvalidatortests.cpp
26 ↗(On Diff #26417)

The comment is now wrong for this case. Either this case is new and deserves its own check or the case is wrong and was changed for some reason.

src/test/cashaddrenc_tests.cpp
72 ↗(On Diff #26417)

Use CASHADDR_PREFIX

This revision now requires changes to proceed.Dec 17 2020, 22:36
test/functional/abc_mining_basic.py
25 ↗(On Diff #26417)

As we discussed offline, this and similar codepaths related to mining will need to be backwards-compatible with the bitcoincash: prefix.

Tail of the build log:

[372/431] Running utility command for check-bitcoin-sync_tests
[373/431] Running utility command for check-bitcoin-sigencoding_tests
[374/431] Running utility command for check-bitcoin-sighashtype_tests
[375/431] Running utility command for check-bitcoin-bip32_tests
[376/431] bitcoin: testing torcontrol_tests
[377/431] Running utility command for check-bitcoin-torcontrol_tests
[378/431] bitcoin: testing timedata_tests
[379/431] bitcoin: testing uint256_tests
[380/431] bitcoin: testing streams_tests
[381/431] bitcoin: testing undo_tests
[382/431] bitcoin: testing wallet_crypto_tests
[383/431] Running utility command for check-bitcoin-timedata_tests
[384/431] Running utility command for check-bitcoin-uint256_tests
[385/431] Running utility command for check-bitcoin-streams_tests
[386/431] Running utility command for check-bitcoin-undo_tests
[387/431] Running utility command for check-bitcoin-wallet_crypto_tests
[388/431] bitcoin: testing util_threadnames_tests
[389/431] bitcoin: testing compilerbug_tests
[390/431] bitcoin: testing validation_chainstatemanager_tests
[391/431] Running utility command for check-bitcoin-util_threadnames_tests
[392/431] bitcoin: testing checkpoints_tests
[393/431] Running utility command for check-bitcoin-compilerbug_tests
[394/431] Running utility command for check-bitcoin-validation_chainstatemanager_tests
[395/431] Running utility command for check-bitcoin-checkpoints_tests
[396/431] bitcoin: testing txvalidationcache_tests
[397/431] bitcoin: testing cuckoocache_tests
[398/431] Running utility command for check-bitcoin-txvalidationcache_tests
[399/431] Running utility command for check-bitcoin-cuckoocache_tests
[400/431] bitcoin: testing validationinterface_tests
[401/431] Running utility command for check-bitcoin-validationinterface_tests
[402/431] bitcoin: testing radix_tests
[403/431] Running utility command for check-bitcoin-radix_tests
[404/431] bitcoin: testing getarg_tests
[405/431] bitcoin: testing ref_tests
[406/431] Running utility command for check-bitcoin-getarg_tests
[407/431] Running utility command for check-bitcoin-ref_tests
[408/431] bitcoin: testing crypto_tests
[409/431] Running utility command for check-bitcoin-crypto_tests
[410/431] bitcoin: testing blockcheck_tests
[411/431] Running utility command for check-bitcoin-blockcheck_tests
[412/431] bitcoin: testing validation_block_tests
[413/431] bitcoin: testing monolith_opcodes_tests
[414/431] bitcoin: testing validation_tests
[415/431] Running utility command for check-bitcoin-validation_block_tests
[416/431] Running utility command for check-bitcoin-monolith_opcodes_tests
[417/431] Running utility command for check-bitcoin-validation_tests
[418/431] bitcoin: testing script_tests
[419/431] Running utility command for check-bitcoin-script_tests
[420/431] bitcoin: testing skiplist_tests
[421/431] Running utility command for check-bitcoin-skiplist_tests
[422/431] bitcoin: testing util_tests
[423/431] Running utility command for check-bitcoin-util_tests
[424/431] bitcoin: testing op_reversebytes_tests
[425/431] Running utility command for check-bitcoin-op_reversebytes_tests
[426/431] bitcoin: testing transaction_tests
[427/431] Running utility command for check-bitcoin-transaction_tests
[428/431] bitcoin: testing coins_tests
[429/431] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1

made 'bitcoincash:' prefix backward compatible for the time being and duplicated a few tests for it

jasonbcox requested changes to this revision.Dec 21 2020, 19:25

abc_mining_basic.py still needs to test that the old prefix is valid for mining

src/test/cashaddrenc_tests.cpp
303 ↗(On Diff #26507)

reusing variables in tests is generally a bad idea

This revision now requires changes to proceed.Dec 21 2020, 19:25
majcosta marked an inline comment as done.

addressed comment on reuse of test variables and added a test to ensure miner fund addresses with both supported prefixes behave the same

Failed tests logs:

====== Bitcoin ABC functional tests: abc_feature_minerfund.py ======

------- Stdout: -------
2020-12-21T20:54:56.772000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20201221_205348/abc_feature_minerfund_238
2020-12-21T20:54:57.047000Z TestFramework (INFO): Create some history
2020-12-21T20:54:57.128000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/abc_feature_minerfund.py", line 97, in run_test
    MINER_FUND_ADDR)
  File "/work/test/functional/test_framework/util.py", line 60, in assert_equal
    for arg in (thing1, thing2) + args)))
AssertionError: not(abcreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgyrm5ege8 == bchreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgd35g0pkl)
2020-12-21T20:54:57.180000Z TestFramework (INFO): Stopping nodes
2020-12-21T20:54:57.482000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20201221_205348/abc_feature_minerfund_238
2020-12-21T20:54:57.482000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20201221_205348/abc_feature_minerfund_238/test_framework.log
2020-12-21T20:54:57.482000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-without-wallet/test/tmp/test_runner_₿₵_  _20201221_205348/abc_feature_minerfund_238' to consolidate all logs

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_feature_minerfund.py

fixed misnamed variable on abc_feature_minerfund.py

Failed tests logs:

====== Bitcoin ABC functional tests: abc_feature_minerfund.py ======

------- Stdout: -------
2020-12-21T20:58:50.853000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201221_205517/abc_feature_minerfund_561
2020-12-21T20:58:51.117000Z TestFramework (INFO): Create some history
2020-12-21T20:58:51.238000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/abc_feature_minerfund.py", line 97, in run_test
    MINER_FUND_ADDR)
  File "/work/test/functional/test_framework/util.py", line 60, in assert_equal
    for arg in (thing1, thing2) + args)))
AssertionError: not(abcreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgyrm5ege8 == bchreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgd35g0pkl)
2020-12-21T20:58:51.289000Z TestFramework (INFO): Stopping nodes
2020-12-21T20:58:51.592000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201221_205517/abc_feature_minerfund_561
2020-12-21T20:58:51.592000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201221_205517/abc_feature_minerfund_561/test_framework.log
2020-12-21T20:58:51.592000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20201221_205517/abc_feature_minerfund_561' to consolidate all logs

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_feature_minerfund.py

Failed tests logs:

====== Bitcoin ABC functional tests: abc_feature_minerfund.py ======

------- Stdout: -------
2020-12-21T20:59:33.891000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205648/abc_feature_minerfund_685
2020-12-21T20:59:34.151000Z TestFramework (INFO): Create some history
2020-12-21T20:59:34.190000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/abc_feature_minerfund.py", line 97, in run_test
    MINER_FUND_ADDR)
  File "/work/test/functional/test_framework/util.py", line 60, in assert_equal
    for arg in (thing1, thing2) + args)))
AssertionError: not(abcreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgyrm5ege8 == bchreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgd35g0pkl)
2020-12-21T20:59:34.241000Z TestFramework (INFO): Stopping nodes
2020-12-21T20:59:34.443000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205648/abc_feature_minerfund_685
2020-12-21T20:59:34.443000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205648/abc_feature_minerfund_685/test_framework.log
2020-12-21T20:59:34.443000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205648/abc_feature_minerfund_685' to consolidate all logs
====== Bitcoin ABC functional tests with the next upgrade activated: abc_feature_minerfund.py ======

------- Stdout: -------
2020-12-21T21:02:18.288000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205943/abc_feature_minerfund_275
2020-12-21T21:02:18.571000Z TestFramework (INFO): Create some history
2020-12-21T21:02:18.643000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 120, in main
    self.run_test()
  File "/work/test/functional/abc_feature_minerfund.py", line 97, in run_test
    MINER_FUND_ADDR)
  File "/work/test/functional/test_framework/util.py", line 60, in assert_equal
    for arg in (thing1, thing2) + args)))
AssertionError: not(abcreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgyrm5ege8 == bchreg:pqnqv9lt7e5vjyp0w88zf2af0l92l8rxdgd35g0pkl)
2020-12-21T21:02:18.694000Z TestFramework (INFO): Stopping nodes
2020-12-21T21:02:18.945000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205943/abc_feature_minerfund_275
2020-12-21T21:02:18.946000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205943/abc_feature_minerfund_275/test_framework.log
2020-12-21T21:02:18.946000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20201221_205943/abc_feature_minerfund_275' to consolidate all logs

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_feature_minerfund.py
Bitcoin ABC functional tests with the next upgrade activated: abc_feature_minerfund.py

jasonbcox requested changes to this revision.Dec 28 2020, 17:49

Tests still failing.

This revision now requires changes to proceed.Dec 28 2020, 17:49
jasonbcox requested changes to this revision.Dec 29 2020, 17:21
jasonbcox added inline comments.
test/functional/abc_feature_minerfund.py
24 ↗(On Diff #26576)

Nit: Call the old one "legacy" and remove "new" from the current one. This is because we should expect to remove the legacy option at some point, but we don't want to call ABC new going forward. This way you don't put off another refactor.

40 ↗(On Diff #26576)

why create new variables here?

This revision now requires changes to proceed.Dec 29 2020, 17:21

yea it does look nicer that way, thanks

deadalnix requested changes to this revision.Jan 2 2021, 20:38
deadalnix added a subscriber: deadalnix.

this patch needs to be several patches.

src/cashaddrenc.cpp
108 ↗(On Diff #26582)

The old and famous if/return:else pattern make a wild appearance.

src/minerfund.cpp
29 ↗(On Diff #26582)

If there is a backward compatible path, then this belongs in another diff.

src/network.h
19 ↗(On Diff #26582)

Now that we do not support BCHN, what purpose does this serve?

test/functional/abc_feature_minerfund.py
24 ↗(On Diff #26582)

This does not belong here.

This revision now requires changes to proceed.Jan 2 2021, 20:38

cut all backwards compatibility changes from the patch, this is a pure prefix change now.

majcosta edited the summary of this revision. (Show Details)

missed some stuff in tests

removed the last backward compatibility tests I missed the first time around.

note: I left in place the tests that hardcode the prefix in cashaddrenc_tests.cpp, for historical reasons and because they don't rely on what the node considers to be the default prefix for a particular chain.

majcosta retitled this revision from [CashAddr] replace bitcoincash: prefix with abc: in the node to [CashAddr] replace bitcoincash: prefix with ecash: in the node.Feb 4 2021, 21:37
majcosta edited the summary of this revision. (Show Details)