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

add spentindex to getrawtransaction RPC results for bitcore block explorer #3863

Merged
merged 1 commit into from Jun 11, 2019

Conversation

@LarryRuane
Copy link
Contributor

commented Mar 5, 2019

#3708 There are a few new getrawtransaction JSON result fields that the Insight block explorer depends on.

@LarryRuane LarryRuane self-assigned this Mar 5, 2019

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

UPDATE 5/8/2019: Previous PRs have been merged, this PR is now down to just one commit.

Reviewers: Please review only commits on or after March 5 (currently only one), as this PR is layered upon several previous PRs which have not yet been merged. My understanding is that adding new results fields to existing RPCs (in this case, to getrawtransaction only if spentindex is enabled) should not affect existing RPC consumers (they just ignore new fields that they don't know or care about), but please check me on this.

There's one aspect that needs special care, so please help me by giving this some thought: I'm not picking up this change:

zcash-hackworks/zcash-patched-for-explorer@7c61470#diff-01aa7d1d32f1b9e5a836c9c411978918R183

because it's likely that many consumers of the getrawtransaction RPC depend on the field valueZat, so it cannot be changed to valueSat. I believe this change was made because the bitcore Insight code expects the field to be valueSat (the various repos contains references to that string, but no references to valueZat). This means that to work with the result of merging the current PR, the Insight code will need to be changed to refer to valueZat (which should be okay because it's specific to Zcash).

An alternative would be to report both valueSat and valueZat in the RPC results (both yielding the same value), but that seems pretty dirty. Let me know if you disagree, or if there's a better way to solve this problem.

@LarryRuane
Copy link
Contributor Author

left a comment

Here are links to both upstream sources (bitpay and zcash-hackworks).

return false;
return pblocktree->ReadSpentIndex(key, value);
}

This comment has been minimized.

out.push_back(Pair("spentTxId", spentInfo.txid.GetHex()));
out.push_back(Pair("spentIndex", (int)spentInfo.inputIndex));
out.push_back(Pair("spentHeight", spentInfo.blockHeight));
}

This comment has been minimized.

Copy link
@str4d

str4d May 30, 2019

Contributor

Interesting side-note: in the latest upstream, they define a new TxToJSONExpanded instead of modifying TxToJSON. Why was that? Maybe just to make maintaining the patchset simpler?

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane Jun 3, 2019

Author Contributor

Yes, I can only think they tried to change as little existing code as possible (preferring instead to add new code). I compared those two functions and they were identical, except the Expanded version added these new fields. Adding these fields to TxToJSON() has the same effect, and reduces the amount of code.

@@ -208,12 +231,14 @@ void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry)
if (mi != mapBlockIndex.end() && (*mi).second) {
CBlockIndex* pindex = (*mi).second;
if (chainActive.Contains(pindex)) {
entry.push_back(Pair("height", pindex->nHeight));
uint256 hash = ParseHashV(params[0], "parameter 1");

bool fVerbose = false;
if (params.size() > 1)
fVerbose = (params[1].get_int() != 0);

LOCK(cs_main);

This comment has been minimized.

Copy link
@str4d

str4d May 30, 2019

Contributor

This is not really needed (given that the other parts of this chunk of the upstream diff are not included), but it's a harmless change.

@LarryRuane LarryRuane changed the title [WIP] add spentindex to getrawtransaction RPC results for bitcore block explorer add spentindex to getrawtransaction RPC results for bitcore block explorer Mar 5, 2019

@LarryRuane LarryRuane requested review from Eirik0 and mdr0id Mar 5, 2019

@LarryRuane LarryRuane marked this pull request as ready for review Mar 5, 2019

@LarryRuane LarryRuane changed the title add spentindex to getrawtransaction RPC results for bitcore block explorer [WIP] add spentindex to getrawtransaction RPC results for bitcore block explorer Mar 7, 2019

@braddmiller braddmiller added this to Needs Verification in Ecosystem Team via automation Mar 13, 2019

@LarryRuane LarryRuane force-pushed the LarryRuane:3708-getrawtransaction branch from b8c0607 to 9dde5f9 Mar 14, 2019

@LarryRuane LarryRuane changed the title [WIP] add spentindex to getrawtransaction RPC results for bitcore block explorer add spentindex to getrawtransaction RPC results for bitcore block explorer Mar 14, 2019

@mms710 mms710 added this to Needs Prioritization in Arborist Team via automation Mar 14, 2019

@mms710 mms710 moved this from Needs Prioritization to R&D Backlog in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from R&D Backlog to Sprint Backlog in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from Sprint Backlog to Sapling Priorities in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from Sapling Priorities to PRs That Need Review + Their Associated Issues in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from PRs That Need Review + Their Associated Issues to Current Sprint in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from Current Sprint to In Progress in Arborist Team Mar 14, 2019

@mms710 mms710 moved this from In Progress to In Review in Arborist Team Mar 14, 2019

@LarryRuane LarryRuane force-pushed the LarryRuane:3708-getrawtransaction branch from 9dde5f9 to 8528c26 Mar 28, 2019

@LarryRuane LarryRuane force-pushed the LarryRuane:3708-getrawtransaction branch 3 times, most recently from bb111ea to c945f5f Apr 5, 2019

@mms710 mms710 moved this from In Review to In Progress in Arborist Team Apr 11, 2019

@mms710 mms710 moved this from In Progress to PRs That Need Review + Their Associated Issues in Arborist Team Apr 11, 2019

@str4d
Copy link
Contributor

left a comment

I checked that the changes all correctly correspond to the linked patchset.

qa/rpc-tests/getrawtransaction_insight.py Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved
uint256 hash = ParseHashV(params[0], "parameter 1");

bool fVerbose = false;
if (params.size() > 1)
fVerbose = (params[1].get_int() != 0);

LOCK(cs_main);

This comment has been minimized.

Copy link
@str4d

str4d May 30, 2019

Contributor

This is not really needed (given that the other parts of this chunk of the upstream diff are not included), but it's a harmless change.

out.push_back(Pair("spentTxId", spentInfo.txid.GetHex()));
out.push_back(Pair("spentIndex", (int)spentInfo.inputIndex));
out.push_back(Pair("spentHeight", spentInfo.blockHeight));
}

This comment has been minimized.

Copy link
@str4d

str4d May 30, 2019

Contributor

Interesting side-note: in the latest upstream, they define a new TxToJSONExpanded instead of modifying TxToJSON. Why was that? Maybe just to make maintaining the patchset simpler?

qa/rpc-tests/getrawtransaction_insight.py Outdated Show resolved Hide resolved
qa/rpc-tests/getrawtransaction_insight.py Outdated Show resolved Hide resolved
qa/rpc-tests/getrawtransaction_insight.py Outdated Show resolved Hide resolved
qa/rpc-tests/getrawtransaction_insight.py Outdated Show resolved Hide resolved
@LarryRuane
Copy link
Contributor Author

left a comment

Thanks, @str4d and @Eirik0 for those good review comments; I think I've addressed all of them. (I didn't force-push but can do so before we merge.)

qa/rpc-tests/getrawtransaction_insight.py Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
out.push_back(Pair("spentTxId", spentInfo.txid.GetHex()));
out.push_back(Pair("spentIndex", (int)spentInfo.inputIndex));
out.push_back(Pair("spentHeight", spentInfo.blockHeight));
}

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane Jun 3, 2019

Author Contributor

Yes, I can only think they tried to change as little existing code as possible (preferring instead to add new code). I compared those two functions and they were identical, except the Expanded version added these new fields. Adding these fields to TxToJSON() has the same effect, and reduces the amount of code.

@LarryRuane LarryRuane force-pushed the LarryRuane:3708-getrawtransaction branch from df174cc to 19cee4a Jun 3, 2019

@LarryRuane LarryRuane removed the request for review from bitcartel Jun 3, 2019

@Eirik0 Eirik0 self-requested a review Jun 3, 2019

@mms710 mms710 requested review from str4d and removed request for mdr0id Jun 4, 2019

bool IsPayToPublicKeyHash() const;
bool IsPayToScriptHash() const;

int Type() const;
boost::optional<ScriptType> GetType() const;

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Jun 4, 2019

Contributor

This looks great! I wonder, could we change enum ScriptType to include NONE = 0. Doing this would allow us to change this to just return ScriptType which I think would simplify this slightly.

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane Jun 6, 2019

Author Contributor

Good suggestion, just pushed a new commit (will squash) to implement your suggestion.

@Eirik0

Eirik0 approved these changes Jun 6, 2019

Copy link
Contributor

left a comment

utACK

@str4d

str4d approved these changes Jun 11, 2019

Copy link
Contributor

left a comment

ut(ACK+cov) modulo squashing the commit that says it will get squashed.

@LarryRuane LarryRuane force-pushed the LarryRuane:3708-getrawtransaction branch from b630e71 to af29c6b Jun 11, 2019

@LarryRuane LarryRuane force-pushed the LarryRuane:3708-getrawtransaction branch from af29c6b to f381d4e Jun 11, 2019

@daira

daira approved these changes Jun 11, 2019

Copy link
Contributor

left a comment

utACK f381d4e (just checking that it does not change consensus code, has tests, and is eligible as a last-minute change before the RC).

@daira

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

zkbot added a commit that referenced this pull request Jun 11, 2019

Auto merge of #3863 - LarryRuane:3708-getrawtransaction, r=daira
add spentindex to getrawtransaction RPC results for bitcore block explorer

#3708 There are a few new `getrawtransaction` JSON result fields that the Insight block explorer depends on.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

⌛️ Testing commit f381d4e with merge 492092b...

@Eirik0 Eirik0 moved this from In Review to Merge Queue in Arborist Team Jun 11, 2019

@Eirik0 Eirik0 added this to the v2.0.6 milestone Jun 11, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

💔 Test failed - pr-merge

@daira

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

This succeeded on Debian 9, so assuming it's nondeterministic. @zkbot retry

zkbot added a commit that referenced this pull request Jun 11, 2019

Auto merge of #3863 - LarryRuane:3708-getrawtransaction, r=daira
add spentindex to getrawtransaction RPC results for bitcore block explorer

#3708 There are a few new `getrawtransaction` JSON result fields that the Insight block explorer depends on.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

⌛️ Testing commit f381d4e with merge bb58c8e...

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 11, 2019

☀️ Test successful - pr-merge
Approved by: daira
Pushing bb58c8e to master...

@zkbot zkbot merged commit f381d4e into zcash:master Jun 11, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) Jun 11, 2019

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Smoke testing:

  • Verify existing behavior (nothing is broken)
    • Start zcashd
    • (the rest of this test assumes you have already synced with the blockchain)
    • Verify that zcash-cli help getrawtransaction looks reasonable (no new fields are included in the help output)
    • Use a block explorer, such as https://explorer.testnet.z.cash/, to examine some transactions
      • Verify that zcash-cli getrawtransaction <txid> 1 gives corresponding results
  • Verify new behavior
    • Stop zcashd
    • Run zcashd with command line option --insightexplorer
      • verify that zcashd stops after printing the error, You need to rebuild the database using -reindex to change -insightexplorer. Please restart with -reindex to recover.
    • Add insightexplorer=1 to .zcash/zcash.conf
    • Run zcashd with no command line arguments
      • verify that zcashd stops after printing the error, You need to rebuild the database using -reindex to change -insightexplorer. Please restart with -reindex to recover.
    • Run zcashd with the command line argument --reindex
      • verify zcashd stops with Error: -insightexplorer requires -txindex.
    • Add txindex=1 to .zcash/zcash.conf
    • Run zcashd -- reindex
      • verify that the client begins syncing with the blockchain
      • wait for the client to sync with the blockchain, which will take several hours for testnet, and about a day for mainnet
    • Run zcash-cli getrawtransaction <txid> 1 on various transactions
      • verify that the expected new fields appear in the results as described in the "Additional getrawtransaction fields" section of the Insight Block Explorer Guide (if not yet in the standard place, https://zcash.readthedocs.io/en/latest/index.html, you can view it here: https://gitlab.com/zcash-docs/zcash-docs/blob/insightexplorer/source/rtd_pages/insight_explorer.rst)
      • correlate these results with the information shown on the block explorer (this will require looking at the linkages between inputs and outputs across multiple transactions)
      • verify that if an output hasn't yet been spent, the spent* fields in thevout part of the getrawtransaction output are not present (you can compare with the (S) (spent) and (U) (unspent) indications to the right of the amounts in the Insight block explorer)
@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Ran through the above suggested testing and everything worked as expected, except I got slightly different error messages in a couple of the steps. I wouldn't really even call this unexpected behavior, but here it is for reference:

  • Verify new behavior

    • Stop zcashd

    • Run zcashd with command line option --insightexplorer

      • verify that zcashd stops after printing the error, You need to rebuild the database using -reindex to change -insightexplorer. Please restart with -reindex to recover.

I got a different error message at this point:

Error: -insightexplorer requires -txindex.

I then tried adding -txindex to zcash.conf, restarted zcashd and got:

Messages:
- : You need to rebuild the database using -reindex to change -txindex.
Please restart with -reindex to
  recover.

I removed -txindex and continued

  • Add insightexplorer=1 to .zcash/zcash.conf

  • Run zcashd with no command line arguments

    • verify that zcashd stops after printing the error, You need to rebuild the database using -reindex to change -insightexplorer. Please restart with -reindex to recover.

I got a different error (consistent with before):

Error: -insightexplorer requires -txindex

Everything from this point on worked as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.