Move CMerkleTx to wallet.cpp/h #4778

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2014_08_cmerkletx_wallet changing 5 files +162 −163

laanwj commented at 3:16 PM on August 28, 2014: member

It is only used by the wallet so it has no place in main.
jgarzik commented at 3:20 PM on August 28, 2014: contributor

ut ACK

Code movement reviewed.
jgarzik commented at 3:21 PM on August 28, 2014: contributor

@laanwj -1?
laanwj force-pushed on Aug 28, 2014
laanwj commented at 3:43 PM on August 28, 2014: member

It now checks whether the block is in the main chain and returns confirmations=-1 if not, as before. Maybe 0 was a better value to return in that case (that changed when the wallet started handling conflicted transactions), that's open for discussion.
laanwj added the label Wallet on Aug 28, 2014
laanwj added the label RPC on Aug 28, 2014

in src/rpcblockchain.cpp:None in db8ed9ec75 outdated

  53 | @@ -54,9 +54,11 @@ Object blockToJSON(const CBlock& block, const CBlockIndex* blockindex)
  54 |  {
  55 |      Object result;
  56 |      result.push_back(Pair("hash", block.GetHash().GetHex()));
  57 | -    CMerkleTx txGen(block.vtx[0]);
  58 | -    txGen.SetMerkleBranch(&block);
  59 | -    result.push_back(Pair("confirmations", (int)txGen.GetDepthInMainChain()));
  60 | +    int confirmations = -1;
  61 | +    // Only report confirmations if the block is on the main chain

sipa commented at 8:43 PM on August 28, 2014:

If chainActive.Contains(blockindex) ?

in src/rpcblockchain.cpp:None in db8ed9ec75 outdated
```
  58 | -    txGen.SetMerkleBranch(&block);
  59 | -    result.push_back(Pair("confirmations", (int)txGen.GetDepthInMainChain()));
  60 | +    int confirmations = -1;
  61 | +    // Only report confirmations if the block is on the main chain
  62 | +    if (chainActive.FindFork(blockindex) == blockindex)
  63 | +        confirmations = chainActive.Height() - blockindex->nHeight + 1;
```
tholenst commented at 2:19 PM on August 29, 2014:

This implies that the current highest block has 1 confirmation and that this RPC call never returns "confirmations: 0". Obviously this cannot be changed (it was previously like this), but maybe one could take this chance and document this behaviour in line 245?

jgarzik commented at 2:26 PM on August 29, 2014:

By definition no block can have zero confirmations... not sure what you're asking? Just to document the "confirmations" field behavior?

tholenst commented at 2:34 PM on August 29, 2014:

Yeah, to document was what I meant.

But you are right -- since the transactions within the block have 1 confirmation, it's only natural that the block also has 1 confirmation. So probably it doens not need more documentation.

laanwj commented at 3:14 PM on August 29, 2014:

That's what I meant above. It returns -1 for blocks that are not on the main chain. This used to be 0 before the wallet conflict changes in 0.9. It looks like this inadvertently changed, with no one realizing that the CMerkleTx was used here.
tholenst commented at 3:10 PM on August 29, 2014: contributor

ACK except for sipa's comment. I verified that the code movement is purely movement, and tested rpcblockchain.
jgarzik commented at 3:13 PM on August 29, 2014: contributor

Indeed. Contains() is slightly more efficient.

rpc: Compute number of confirmations of a block from block height

Currently this uses a CMerkleTx, but that makes no sense as we
have the CBlockIndex available. As noted by @jgarzik.

57153d4e1a

Move CMerkleTx to wallet.cpp/h

It is only used by the wallet so it has no place in main.

0101483f46

laanwj force-pushed on Aug 29, 2014
laanwj commented at 3:27 PM on August 29, 2014: member

Changed to @sipa's suggestion, changed documentation for getblock to mention that -1 confirmations is returned for blocks not on the main chain.
BitcoinPullTester commented at 3:40 PM on August 29, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4778_0101483f46396a7f1d19a9d29a1da15639ce4233/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
sipa commented at 4:11 PM on August 29, 2014: member

Untested ACK. Verified move-only.
jgarzik merged this on Aug 30, 2014
jgarzik closed this on Aug 30, 2014
jgarzik referenced this in commit 135a43df7b on Aug 30, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels