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

rpc: implement gettxout JSON-RPC method for SPV mode #1903

Merged
merged 3 commits into from May 25, 2021

Conversation

itswisdomagain
Copy link
Member

@itswisdomagain itswisdomagain commented Oct 27, 2020

In RPC mode, the method performs passthrough manually, to keep
existing behavior.

In SPV mode, only wallet-related unspent outputs can be looked up
currently.

spv/sync.go Outdated Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Looks good, but untested. I just have a few thoughts, but nothing to change except a go generate.

internal/rpc/jsonrpc/methods.go Outdated Show resolved Hide resolved
internal/rpc/jsonrpc/methods.go Outdated Show resolved Hide resolved
internal/rpc/jsonrpc/methods.go Outdated Show resolved Hide resolved
internal/rpc/jsonrpc/methods.go Outdated Show resolved Hide resolved
internal/rpc/jsonrpc/methods.go Outdated Show resolved Hide resolved
internal/rpc/jsonrpc/methods.go Show resolved Hide resolved
"gettxoutresult-confirmations": "The number of confirmations",
"gettxoutresult-value": "The transaction amount in DCR",
"gettxoutresult-scriptPubKey": "The public key script used to pay coins as a JSON object",
"gettxoutresult-version": "The transaction version",
Copy link
Member

Choose a reason for hiding this comment

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

The transaction version isn't set in our result at all. There is no field for it, but it would be useful.

Should this even be the transaction version, and not the script version? Or should we include both?

I went to check dcrd to see what it used for this, and it also has this same help text describing a transaction version, but the result similarly has no version field.

cc @davecgh

Copy link
Member

Choose a reason for hiding this comment

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

It used to be the transaction version, but that was removed by @rstaudt2 in decred/dcrd@d05271c. It looks like the help entry wasn't removed as well though.

Copy link
Member

@davecgh davecgh May 7, 2021

Choose a reason for hiding this comment

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

That aside, I think the RPC should be updated to return a version field under scriptPubKey for the script version. This is going to become important when we introduce a new version. I'll open an issue on dcrd to add that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see decred/dcrd#2650 is already open to add scriptPubKey.version for dcrd. Will need to bump github.com/decred/dcrd/rpc/jsonrpc/types/v3 to v4/master (after the dcrd pr is merged) to be able to set that value in wallet. Any plans to bump dcrd deps versions for wallet soon @jrick? I can open a PR for that if no one else is available to get on it sooner; I suspect I'll need the deps updated for something else on dcrdex.

Copy link
Member

Choose a reason for hiding this comment

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

dep bump is happening soon. I have a half finished diff sitting in my repo.

In RPC mode, the method performs passthrough manually, to keep
existing behavior.

In SPV mode, only wallet-related unspent outputs can be looked up
currently.
@jrick jrick merged commit fd019dc into decred:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants