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.

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

[INDEX] Index cold staking UTXOs by spending address #731

Merged

Conversation

aguycalled
Copy link
Member

This PR changes the behaviour of the spending index, indexing utxos by the spending address in case of cold staking outputs.

Test in devnet:

> sendtoaddress n4Hk2BvuqUb9pSw7btqgeYMkuVi3iPfrEr 1
> generate 1
> getaddressutxos '{"addresses":["n4Hk2BvuqUb9pSw7btqgeYMkuVi3iPfrEr"]}'
[
  {
    "address": "n4Hk2BvuqUb9pSw7btqgeYMkuVi3iPfrEr",
    "txid": "a75d396753878a99d86a1b751618a61da20f569827e91658381d887722002938",
    "outputIndex": 1,
    "script": "76a914f9cb3d76f4d82188af99851e03d8472767ea0d7688ac",
    "satoshis": 100000000,
    "height": 301
  }
]
> getcoldstakingaddress n2BSoobWfxkMcVUSghd6hMCVACNDSxxNmN n4Hk2BvuqUb9pSw7btqgeYMkuVi3iPfrEr
2agWjSDvB8bwAUSsTfF9qBDVB7KJcfCn64jqiAZp2EoZzqtQ4TXdfnBhhNksyi
> sendtoaddress 2agWjSDvB8bwAUSsTfF9qBDVB7KJcfCn64jqiAZp2EoZzqtQ4TXdfnBhhNksyi 1
> generate 1
> getaddressutxos '{"addresses":["n4Hk2BvuqUb9pSw7btqgeYMkuVi3iPfrEr"]}'
[
  {
    "address": "n4Hk2BvuqUb9pSw7btqgeYMkuVi3iPfrEr",
    "txid": "a75d396753878a99d86a1b751618a61da20f569827e91658381d887722002938",
    "outputIndex": 1,
    "script": "76a914f9cb3d76f4d82188af99851e03d8472767ea0d7688ac",
    "satoshis": 100000000,
    "height": 301
  },
  {
    "address": "n4Hk2BvuqUb9pSw7btqgeYMkuVi3iPfrEr",
    "txid": "5c921072c20b290d529602f48c13207d39af2ecc1c121c433ed207192cb61d73",
    "outputIndex": 1,
    "script": "c66376a914e2aa1fa5b14f932381761c893d1c92711655b8ae88ac6776a914f9cb3d76f4d82188af99851e03d8472767ea0d7688ac68",
    "satoshis": 100000000,
    "height": 302
  }
]

@navbuilder
Copy link

A new build of c5d657f has completed succesfully!
Binaries available at https://build.nav.community/binaries/spentindex-coldstaking-spending

@mxaddict
Copy link
Contributor

Changes make sense, will test once I get to my PC

Copy link
Contributor

@mxaddict mxaddict left a comment

Choose a reason for hiding this comment

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

Test scenario seems to be working, but I think it also works on master branch. Can someone else test master branch to make sure I'm still sane? Haha.

@aguycalled
Copy link
Member Author

@sakdeniz

@mxaddict
Copy link
Contributor

mxaddict commented Aug 7, 2020

@sakdeniz can you confirm this fixes your issue?

@sakdeniz
Copy link

sakdeniz commented Aug 7, 2020

@sakdeniz can you confirm this fixes your issue?

I could not see a problem in my tests.

@chasingkirkjufell
Copy link
Contributor

I am getting "unknown address type" for cold-staking v2. Is the RPC command compatible with cold-staking v2?

@chasingkirkjufell
Copy link
Contributor

I guess this command is for normal addresses, but I don't see a difference in command output compared to current master. What's the expected change from this PR?

@aguycalled
Copy link
Member Author

aguycalled commented Aug 12, 2020

@sakdeniz reported this issue originally, so probably he can describe what happened better.. From what I remember, without this patch, coins sent to a cold staking address were accounted for the staking address instead of the spending address in the spending index. The spending index tracks the UTXO set. With this pr's path applied, outputs sent to a cold staking address should not show in the output of getaddressutxos for the staking address, but should show for the spending address.

Copy link
Contributor

@chasingkirkjufell chasingkirkjufell left a comment

Choose a reason for hiding this comment

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

Tested gitian build on Windows 10

@aguycalled aguycalled merged commit 4c2264c into navcoin:master Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants