Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Test] Print error if shield diversified address is equal to the default address #2116

Merged
merged 1 commit into from
Dec 31, 2020
Merged

[Test] Print error if shield diversified address is equal to the default address #2116

merged 1 commit into from
Dec 31, 2020

Conversation

furszy
Copy link

@furszy furszy commented Dec 30, 2020 •
edited

Trying to determine the inconsistent failing, linux-only, unit test case.

The hypothesis is that the diversified address is equal to the the default address (empty diversified).
As I haven't been able to replicate it locally, this PR is printing the error if happens in GA. The error is the following:

 Test cases order is shuffled using seed: 1449329889
2325
  Entering test module "Pivx Test Suite"
2326
  test/librust/wallet_zkeys_tests.cpp(23): Entering test suite "wallet_zkeys_tests"
2327
  test/librust/wallet_zkeys_tests.cpp(122): Entering test case "WriteCryptedSaplingZkeyDirectToDb"
2328
  test/librust/wallet_zkeys_tests.cpp(122): Leaving test case "WriteCryptedSaplingZkeyDirectToDb"; testing time: 1675295us
2329
  test/librust/wallet_zkeys_tests.cpp(32): Entering test case "StoreAndLoadSaplingZkeys"
2330
  test/librust/wallet_zkeys_tests.cpp(88): error: in "wallet_zkeys_tests/StoreAndLoadSaplingZkeys": check !wallet.HaveSaplingIncomingViewingKey(dpa) has failed
2331
  test/librust/wallet_zkeys_tests.cpp(32): Leaving test case "StoreAndLoadSaplingZkeys"; testing time: 360812us
2332
  test/librust/wallet_zkeys_tests.cpp(23): Leaving test suite "wallet_zkeys_tests"; testing time: 2036175us
2333
  Leaving test module "Pivx Test Suite"; testing time: 2036270us

Aside from that, have to mention that this isn't something that will affect/delay mainnet release in any way.
We are not using diversified addresses at all in the wallet and even if we add this feature in the future, the address is already on the wallet, so it's simply matter of verify if it's in the wallet and move to the next diversified in the path if we already have the address loaded.

@furszy furszy self-assigned this Dec 30, 2020
random-zebra
random-zebra approved these changes Dec 30, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK eb28c9f

@random-zebra random-zebra added this to the 5.0.0 milestone Dec 30, 2020
@random-zebra random-zebra added the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Dec 30, 2020
Fuzzbawls
Fuzzbawls approved these changes Dec 31, 2020
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK eb28c9f

@furszy furszy merged commit 4e438d8 into PIVX-Project:master Dec 31, 2020
@furszy furszy mentioned this pull request Jan 4, 2021
Merged
random-zebra added a commit that referenced this pull request Jan 5, 2021
@random-zebra
Merge #2123: [Test] Attempt to fix the diversified address equal to d…
9d935a9 
…efault address test scenario

910b580 Test case: StoreAndLoadSaplingZkeys, added a loop to try to generate a diversified address different than the default one. (furszy)

Pull request description:

  Follow up investigation to #2116. Based on GA results, the diversified address is equal to the default address.
  The code now will try to create a different address to the default one in a loop increasing the diversifier by one on each round.

  This issue will need a deeper investigation in the future if/when we add diversified addresses features (at this moment, is pretty irrelevant as we aren't using diversified addresses at all). I have seen it only happening randomly on GA on linux and doesn't seems to be a library issue, if we provide the same seed to the process, the error isn't being thrown. It seems more to be an issue with the data pointer.

ACKs for top commit:
  random-zebra:
    utACK 910b580

Tree-SHA512: 8be1c093c63c648741f7cf2b1179e41314b1b77daf418b0df042996a19085d81db078aae563c51926b8534b99ebbe9930badf23ecd65a81ae255f3500bf95fe4
furszy referenced this pull request in furszy/bitcoin-core Jan 5, 2021
@furszy
Test: print error if shield diversified address is equal to the defau…
51e1c97 
…lt address.

Github-Pull: bitcoin#2116
Rebased-From: eb28c9f
@furszy furszy mentioned this pull request Jan 5, 2021
Merged
Fuzzbawls added a commit that referenced this pull request Jan 6, 2021
@Fuzzbawls
Merge #2124: [Backport] Final 5.0 backports
f4df550 
41fb982 Add PR list and authors to release notes for v5.0.0 (Fuzzbawls)
528912b [Doc] release-notes: final enforcement block height plus timeline recommendation. (furszy)
a778833 [Doc] Add "Mandatory Upgrade" section (ActiveProtocol upgrade time TBD) Github-Pull: #2114 Rebased-From: 8f9e1d9 (random-zebra)
0c2dc8d [Doc] Document HD wallet upgrade to SAPLING_VERSION Github-Pull: #2114 Rebased-From: c94c19e (random-zebra)
ff59122 [Doc] Document SwiftX and Zerocoin removal Github-Pull: #2114 Rebased-From: d36f37f (random-zebra)
0e37c67 [Doc] Document consensus and policy changes related to Shield Github-Pull: #2114 Rebased-From: cc3c83d (random-zebra)
07d9db7 [Doc] Document more RPC changes Github-Pull: #2114 Rebased-From: 08b50bb (random-zebra)
95992ac [Doc] minor changes to the "Shield Protocol" section of the relnotes Github-Pull: #2114 Rebased-From: 0f3f77d (random-zebra)
75a7597 [Doc] Document new dependencies (sodium/rust) Github-Pull: #2114 Rebased-From: 39ee780 (random-zebra)
033e8b3 [Doc] Document sapling parameters for installation/upgrade Github-Pull: #2114 Rebased-From: 1d18ebf (random-zebra)
a3d2b52 Add RPC changes to the release notes (Fuzzbawls)
b2b113c [Doc] v5 release notes (furszy)
3fd22bf [GUI] Update Translations from Transifex (Fuzzbawls)
78d0b01 Test case: StoreAndLoadSaplingZkeys, added a loop to try to generate a diversified address different than the default one. (furszy)
090b901 [RPC][Wallet] Update also Sapling HD seed with sethdseed Github-Pull: #2120 Rebased-From: 092f825 (random-zebra)
36336ec [Tests] Verify Sapling wallet HD chain in wallet_hd.py Github-Pull: #2120 Rebased-From: e14fe3d (random-zebra)
d8d347e [RPC] Add shield address support to getaddressinfo Github-Pull: #2120 Rebased-From: 8ec1875 (random-zebra)
9eb84ce [Cleanup][RPC] Remove not-returned fields from getaddressinfo help Github-Pull: #2120 Rebased-From: 501782a (random-zebra)
0d8b6c0 Cleaning up not used `nWalletDBUpdated`. (furszy)
6219dfe [GUI] fixing tooltip balance amount cropped when the value surpass the 1kk range. (furszy)
0e158b0 RPC: Document the `services` return entry in `getinfo` (Fuzzbawls)
576efe8 RPC: Properly document all example return fields in getchaintips (Fuzzbawls)
7100c33 RPC: Fix help output for getblockchaininfo (Fuzzbawls)
3254dc8 RPC: Fix help output for waitfor* commands (Fuzzbawls)
46f47c5 RPC: document results for dumpwallet (Fuzzbawls)
3bfb513 RPC: drop "ed" suffix from input arguments for shield export commands (Fuzzbawls)
9bc65af RPC: document per-network MN count for getmasternodecount (Fuzzbawls)
aaf9216 RPC: Remove unused mnEntries vector (Fuzzbawls)
90b1d6e [GUI][RPC] Add exportsaplingkey to the list of pot. dangerous cmds Github-Pull: #2122 Rebased-From: 81e4d15 (random-zebra)
55ff95e [GUI][BUG] Fix confirmation dialog for dumpwallet/dumpprivkey (random-zebra)
8863d76 [GUI][Trivial] Save cold-staking address in contacts (random-zebra)
51e1c97 Test: print error if shield diversified address is equal to the default address. (furszy)
351b2ce [Refactor] CoinControl: getTotals returns struct Github-Pull: #2111 Rebased-From: efa1331 (random-zebra)
6506d71 [BUG] Fix isDust label in coincontrol Github-Pull: #2111 Rebased-From: 0e08e40 (random-zebra)
805552c [Refactor] CControl: decouple getTotals from updateLabels Github-Pull: #2111 Rebased-From: 8e79aa9 (random-zebra)
391eff6 [GUI] Fix size computation in coin-control (random-zebra)
1656246 [Doc] RPC getrawtransaction and decoderawtransaction, sapling transaction data (furszy)
1ed9970 Set v5 enforcement time for mainnet (furszy)
0cf5703 [GUI] Support for proposal outputs (op_return). (furszy)

Pull request description:

  Back porting PRs from master:

  #2100
  #2111
  #2112
  #2113
  #2114
  #2115
  #2116
  #2119
  #2120
  #2121
  #2122
  #2123
  #2125

ACKs for top commit:
  random-zebra:
    utACK 41fb982
  Fuzzbawls:
    ACK 41fb982

Tree-SHA512: da8706da289ea7f0b09fc6fa39bd55579d25cb6b0761ac9999ccf6609cd68fe54bd5b325d9c686758b78b234397eb1e1d0091e69f663110f695899f8fa7c3c7a
@random-zebra random-zebra removed the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Jan 8, 2021
@furszy furszy deleted the 2020_test_print_diversified_addr_error branch November 29, 2022 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

@random-zebra random-zebra random-zebra approved these changes

Assignees

@furszy furszy

Labels
Tests
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@furszy @Fuzzbawls @random-zebra