[rpc] listsinceblock should include lost transactions when parameter is a reorg'd block

kallewoof commented at 5:41 am on January 24, 2017: member

The following scenario will not notify the caller of the fact tx0 has been dropped:

User 1 receives BTC in tx0 from utxo1 in block aa1.
User 2 receives BTC in tx1 from utxo1 (same) in block bb1
User 1 sees 2 confirmations at block aa3.
Reorg into bb chain.
User 1 asks listsinceblock aa3 and does not see that tx0 is now invalidated.

See listsinceblock.py commit for related test.

The proposed fix is to iterate from the given block down to the fork point, and to check each transaction in the blocks against the wallet, in addition to including all transactions from the fork point to the active chain tip (the current behavior). Any transactions that were present will now also be listed in the listsinceblock output in a new replaced array. This operation may be a bit heavy but the circumstances (and perceived frequency of occurrence) warrant it, I believe.

Example output:

 0{
 1  'transactions': [],
 2  'replaced': [
 3    {
 4      'walletconflicts': [],
 5      'vout': 1,
 6      'account': '',
 7      'timereceived': 1485234857,
 8      'time': 1485234857,
 9      'amount': '1.00000000',
10      'bip125-replaceable': 'unknown',
11      'trusted': False,
12      'category': 'receive',
13      'txid': 'ce673859a30dee1d2ebdb3c05f2eea7b1da54baf68f93bb8bfe37c5f09ed22ff',
14      'address': 'miqEt4kWp9zSizwGGuUWLAmxEcTW9bFUnQ',
15      'label': '',
16      'confirmations': -7
17    }
18  ],
19  'lastblock': '7a388f27d09e3699102a4ebf81597d974fc4c72093eeaa02adffbbf7527f6715'
20}

I believe this addresses the comment by @luke-jr in #9516 (comment) but I could be wrong..

jonasschnelli added the label RPC/REST/ZMQ on Jan 24, 2017

jonasschnelli added the label Wallet on Jan 24, 2017

jonasschnelli commented at 8:01 am on January 24, 2017: contributor

I’m not 100% sure if we want this. If we, we would need at least to updated the listsinceblock’s RPC help message to mention that if one requests tx’s from a block outside the main-chain, that the result will also contain transactions not in the main chain.

kallewoof commented at 8:11 am on January 24, 2017: member

@jonasschnelli Updated the help output. I am not sure what the reasons for not wanting this are, unless you’re referring to resource consumption. I think it’s a rare (but important) enough case to warrant it.

It would definitely make it easier for RPC applications checking the validity of existing transactions to explicitly provide these when a reorg affects them. The only other alternative right now is to keep a list of transactions with confirmations less than some arbitrary number (100) and to loop through these every time a reorg is encountered to ensure they’re actually still present.

Edit: one concern of my own is whether a naive implementation would ignore the confirmations value and simply think the transaction existed in the chain, even though the opposite is the case. I wondered if maybe a different key for the returned results should be used, e.g. “reorged” or something. I.e. you would get {"transactions": [list of txs that changed], "reorged": [list of txs that disappeared], "lastblock": <hash>}…

jonasschnelli commented at 12:59 pm on January 24, 2017: contributor

@kallewoof: I missed the point that if you pass in an orphan block to list since, you also get the transactions upwards the chain-fork on the main chain. At first sight, I though you get only tx from the re-orged-off chain in that case.

Concept ACK (and I think it would be clever to list them in an extra array element reorged:[].

kallewoof commented at 1:24 pm on January 24, 2017: member

@jonasschnelli Gotcha. I updated the OP to clarify that it’s also including transactions from the fork point to the active chain tip. I also moved the off chain transactions into a new ‘reorged’ array. (f501acc & 461d5a3)

in src/wallet/rpcwallet.cpp: in fff5875531 outdated

1741+        }
1742+        for (const CTransactionRef& tx : block.vtx)
1743+        {
1744+            if (pwalletMain->mapWallet.count(tx->GetHash()))
1745+            {
1746+                ListTransactions(pwalletMain->mapWallet[tx->GetHash()], "*", -depth, true, transactions, filter);

ryanofsky commented at 9:03 pm on January 24, 2017:

I think it would be helpful to add a comment about -depth here. I was staring at this a long time to figure out how it worked. Comment could say: “Pass -depth as minDepth to prevent any filtering in ListTransactions. (Works because tx can only conflict with transactions after pindex, so GetDepthInMainChain will always return at least (1-depth)).”

in src/wallet/rpcwallet.cpp: in 0c3d1f56f3 outdated

1640@@ -1641,7 +1641,9 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1641     if (request.fHelp)
1642         throw runtime_error(
1643             "listsinceblock ( \"blockhash\" target_confirmations include_watchonly)\n"
1644-            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n"
1645+            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted.\n"
1646+            "If \"blockhash\" is no longer a part of the main chain, all transactions affecting the node wallet back to the fork point are included, as well as those from \n"

ryanofsky commented at 9:14 pm on January 24, 2017:

Would s/back to the fork point/from blockhash back to the fork point/ to clarify, because it sounds to me like this is referring to transactions between the active tip and the fork point (making the rest of the sentence confusing).

in src/wallet/rpcwallet.cpp: in f501acc7b3 outdated

1756@@ -1756,6 +1757,7 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1757 
1758     UniValue ret(UniValue::VOBJ);
1759     ret.push_back(Pair("transactions", transactions));
1760+    ret.push_back(Pair("reorged", reorged));

ryanofsky commented at 9:16 pm on January 24, 2017:

Need documentation update to accompany this.

in qa/rpc-tests/listsinceblock.py: in 461d5a37a3 outdated

117+        utxos = self.nodes[2].listunspent()
118+        utxo = utxos[0]
119+        privkey = self.nodes[2].dumpprivkey(utxo['address'])
120+        self.nodes[1].importprivkey(privkey)
121+
122+        # send from nodes[2] using utxo to nodes[0]

ryanofsky commented at 9:26 pm on January 24, 2017:

Really sending from node 1 here (even if key originally comes from node 2)

in qa/rpc-tests/listsinceblock.py: in 461d5a37a3 outdated

152+        self.sync_all()
153+
154+        self.join_network()
155+
156+        # gettransaction should work for txid1
157+        tmp = self.nodes[0].gettransaction(txid1)

ryanofsky commented at 9:27 pm on January 24, 2017:

unused variable

in qa/rpc-tests/listsinceblock.py: in 461d5a37a3 outdated

156+        # gettransaction should work for txid1
157+        tmp = self.nodes[0].gettransaction(txid1)
158+
159+        # listsinceblock(lastblockhash) should now include txid1, as seen from nodes[0]
160+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
161+        found = False

ryanofsky commented at 9:34 pm on January 24, 2017:

Maybe condense this block to a single line, and add a check for txid2 (untested)

0assert_equal(any(tx['txid'] == txid1 for tx in lsbres['reorged']), True)
1assert_equal(any(tx['txid'] == txid2 for tx in lsbres['transactions']), True)

kallewoof commented at 4:56 am on January 25, 2017:

@ryanofsky Ahh, nice! I didn’t know about any().

Since txid2 is not related to nodes[0], it will not list it anywhere so that second assert_equal will not be true.

Thanks for all the feedback! I believe everything you suggested is in 9caa0ec & 131df5a.

ryanofsky approved

ryanofsky commented at 9:35 pm on January 24, 2017: member

utACK 461d5a37a3d83edbeedb701ed207bc14412dee0d with minor suggestions

kallewoof force-pushed on Jan 25, 2017

MarcoFalke added this to the milestone 0.14.0 on Jan 25, 2017

in qa/rpc-tests/listsinceblock.py: in 131df5adeb outdated

81+        This tests the case where the same UTXO is spent twice on two separate
82+        blocks as part of a reorg.
83+
84+             ab0
85+          /       \
86+        aa1 [tx0]   bb1 [tx1]

ryanofsky commented at 3:18 pm on January 25, 2017:

Maybe change tx0 and tx1 to tx1 and tx2 to match code. Also code is generating 6 and 7 blocks instead of 3 and 4, so maybe that could be changed to match the example.

in src/wallet/rpcwallet.cpp: in 131df5adeb outdated

1748+        for (const CTransactionRef& tx : block.vtx)
1749+        {
1750+            if (pwalletMain->mapWallet.count(tx->GetHash()))
1751+            {
1752+                // Use -depth as minDepth parameter to ListTransactions, as these transactions have
1753+                // negative (at minimum -depth) confirmations.

ryanofsky commented at 3:28 pm on January 25, 2017:

Would remove parentheses since (at minimum -depth) is the important part. Also, technically I believe the minimum is 1-depth. Maybe:

0// Use -depth as minDepth parameter to ListTransactions to prevent incoming
1// transactions from being filtered. These transactions have negative
2// confirmations, but always greater than -depth.

kallewoof commented at 10:47 pm on January 25, 2017: member

@ryanofsky Thanks for all the feedback! Updated.

gmaxwell commented at 7:19 am on January 26, 2017: contributor

Concept ACK.

kallewoof force-pushed on Jan 27, 2017

in src/wallet/rpcwallet.cpp: in d92496cd4d outdated

1750+            if (pwalletMain->mapWallet.count(tx->GetHash()))
1751+            {
1752+                // Use -depth as minDepth parameter to ListTransactions to prevent incoming
1753+                // transactions from being filtered. These transactions have negative
1754+                // confirmations, but always greater than -depth.
1755+                ListTransactions(pwalletMain->mapWallet[tx->GetHash()], "*", -depth, true, reorged, filter);

TheBlueMatt commented at 8:09 pm on January 28, 2017:

I dont believe this prevents listing transactions which were on both sides of the fork? I’m pretty sure we dont want to list such transactions in a “reorged” list.

TheBlueMatt commented at 8:09 pm on January 28, 2017: member

I’m not sure I’d call this a bugfix - we already correctly print transactions which were included in blocks from the common fork point from the blockhash provided to listsinceblock.

kallewoof commented at 1:59 am on January 30, 2017: member

@TheBlueMatt I guess it comes down to what the user expects. I personally expected listsinceblock to show me anything I needed to keep an updated score of what’s going on since the given block. In that sense, not showing a transaction vanishing would probably be considered a bug.

Regarding showing an existing transaction in both reorged and transactions, I patched this by ensuring that any tx shown from the main chain is skipped over in the reorged list. See the new test_double_send test.

kallewoof force-pushed on Jan 30, 2017

JeremyRubin commented at 8:45 am on January 30, 2017: contributor

I agree with Matt, this isn’t a bugfix, it’s feature :)

I think I’m in favor of adding this. I would be more supportive of naming them replaced rather than reorged if I understand the semantics properly. Strictly speaking, I’d say a reorged txn is any transaction that was between the old tip and the fork point?

I would also think maybe making this information available with a default false parameter may be a good idea to introduce less changes for current users/legacy code.

kallewoof renamed this:
~~[rpc] Bug-fix: listsinceblock should include lost transactions when parameter is a reorg'd block~~
[rpc] listsinceblock should include lost transactions when parameter is a reorg'd block
on Jan 30, 2017

kallewoof commented at 9:05 am on January 30, 2017: member

@JeremyRubin Changed as you said, except setting default of include_replaced to true as I think that will be the most useful case.

kallewoof force-pushed on Jan 30, 2017

laanwj added this to the milestone 0.15.0 on Jan 30, 2017

laanwj removed this from the milestone 0.14.0 on Jan 30, 2017

laanwj commented at 11:32 am on January 30, 2017: member

Moving milestone to 0.15 as this was re-classified as a feature and the feature freeze is long past.

kallewoof force-pushed on Jan 30, 2017

in src/wallet/rpcwallet.cpp: in a7d1e2a37d outdated

1665+            "In addition, transactions from the fork point up to the tip of the active chain are included in \"transactions\".\n"
1666             "\nArguments:\n"
1667             "1. \"blockhash\"            (string, optional) The block hash to list transactions since\n"
1668             "2. target_confirmations:    (numeric, optional) The confirmations required, must be 1 or more\n"
1669             "3. include_watchonly:       (bool, optional, default=false) Include transactions to watch-only addresses (see 'importaddress')"
1670+            "4. include_replaced:        (bool, optional, default=true) Show transactions that were replaced due to a reorg in the \"replaced\" array"

luke-jr commented at 8:53 pm on February 2, 2017:

Not sure this needs to be optional…?

kallewoof commented at 9:14 pm on February 2, 2017:

Me neither. It was mostly suggested as a way for legacy code to cope better, but only really crappy legacy code (that would break when a new key was added to an existing dictionary) would be affected I guess.

in src/wallet/rpcwallet.cpp: in a7d1e2a37d outdated

1753         CWalletTx tx = (*it).second;
1754 
1755         if (depth == -1 || tx.GetDepthInMainChain() < depth)
1756-            ListTransactions(tx, "*", 0, true, transactions, filter);
1757+        {
1758+            if (ListTransactions(tx, "*", 0, true, transactions, filter) && include_replaced)

luke-jr commented at 9:00 pm on February 2, 2017:

Why do we need ListTransactions’s return value? If it doesn’t get inserted here, it won’t later-on either, right?

kallewoof commented at 9:15 pm on February 2, 2017:

See above case with a tx being sent on both chains.

in src/wallet/rpcwallet.cpp: in a7d1e2a37d outdated

1346+ * @param  fLong      Whether to include the JSON version of the transaction.
1347+ * @param  ret        The UniValue into which the result is stored.
1348+ * @param  filter     The "is mine" filter bool.
1349+ * @return            true if appends were made to ret, false otherwise.
1350+ */
1351+bool ListTransactions(const CWalletTx& wtx, const string& strAccount, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter)

luke-jr commented at 9:01 pm on February 2, 2017:

IMO this PR doesn’t need to modify ListTransactions, and this ought to be move to a separate PR.

kallewoof commented at 9:13 pm on February 2, 2017:

For the case where tx0 is in reorged and main chain both (see test_double_send), both versions of the transaction will appear in the results (once in ’transactions’ and once in ‘replaced’). To prevent that, I check if ListTransactions appended to the transactions array – if it did, I need to not include in replaced.

luke-jr approved

luke-jr commented at 9:01 pm on February 2, 2017: member

utACK as-is, with a few nits. (IMO this is a bug fix.)

luke-jr commented at 7:45 am on February 10, 2017: member

It occurs to me that the key "replaced" may not be ideal, since there is no guarantee these transactions are now conflicted and won’t get re-mined. Perhaps "removed"?

kallewoof commented at 8:22 am on February 10, 2017: member

I tried to find actual cases where this happened, but had a hard time finding a case that wasn’t the cause of a double spend. It would require that the transaction is NOT in the mempool of the node, or it would appear as normal in transactions. There may be a case I haven’t thought of though, so you may be right, removed is a safer bet for sure.

kallewoof force-pushed on Feb 10, 2017

in src/wallet/rpcwallet.cpp: in 464f557da9 outdated

1670@@ -1654,12 +1671,15 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1671 
1672     if (request.fHelp)
1673         throw runtime_error(
1674-            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly)\n"
1675-            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n"
1676+            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly include_replaced )\n"

luke-jr commented at 9:18 am on February 10, 2017:

“replaced”

kallewoof commented at 2:20 pm on February 10, 2017:

Done

in src/wallet/rpcwallet.cpp: in 464f557da9 outdated

1670@@ -1654,12 +1671,15 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1671 
1672     if (request.fHelp)
1673         throw runtime_error(
1674-            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly)\n"
1675-            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n"
1676+            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly include_replaced )\n"
1677+            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted.\n"
1678+            "If \"blockhash\" is no longer a part of the main chain, all transactions affecting the node wallet from blockhash back to the fork point are included in the \"replaced\" array.\n"

luke-jr commented at 9:18 am on February 10, 2017:

“replaced”

kallewoof commented at 2:20 pm on February 10, 2017:

Done

luke-jr changes_requested

luke-jr commented at 9:19 am on February 10, 2017: member

Two more “replaced” instances. Also need to update the named param list at the bottom of the file, and in rpc/client.cpp

kallewoof commented at 2:21 pm on February 10, 2017: member

@luke-jr Sorry about sloppiness, I thought I got all of them in the initial two squashme’s.

kallewoof force-pushed on Feb 15, 2017

ryanofsky commented at 9:57 pm on February 23, 2017: member

utACK ad57cefb6583ab669d8da21d0bf808e2d38ef04a.

Comparing against previously ACKed 461d5a37a3d83edbeedb701ed207bc14412dee0d, the both-sides dedup code and the new API tweaks and comments looked good.

in src/wallet/rpcwallet.cpp: in ad57cefb65 outdated

1670@@ -1654,12 +1671,15 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1671 
1672     if (request.fHelp)
1673         throw runtime_error(
1674-            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly)\n"
1675-            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n"
1676+            "listsinceblock ( \"blockhash\" target_confirmations include_watchonly include_removed )\n"
1677+            "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted.\n"
1678+            "If \"blockhash\" is no longer a part of the main chain, all transactions affecting the node wallet from blockhash back to the fork point are included in the \"removed\" array.\n"

TheBlueMatt commented at 4:05 pm on February 24, 2017:

Maybe instead of this and the next line, “If "blockhash" is no longer a part of the main chain, transactions from the fork point onward are included.\n” “Additionally, if include_removed is set, transactions affecting the wallet which were removed are returned in the "removed" array.\n”

kallewoof commented at 5:36 pm on February 24, 2017:

Makes sense. Changed.

in src/wallet/rpcwallet.cpp: in ad57cefb65 outdated

1782+    while (include_removed && paltindex && paltindex != pindex)
1783+    {
1784+        CBlock block;
1785+        if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus()))
1786+        {
1787+            throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");

TheBlueMatt commented at 4:20 pm on February 24, 2017:

Hmm, not sure how I feel about this. For pruning nodes, it may be more useful to return a partial list and a message indicating the results are incomplete rather than throw wholesale.

kallewoof commented at 5:49 pm on February 24, 2017:

Isn’t that slightly dangerous? I mean, if people don’t realize the node only has a subset of all blocks in some cases, they might miss the “partial” flag and never realize they potentially missed something. Even if it’s their own node. Someone could even time an attack based on that.

To take a step back, the situation here is that some software which uses listsinceblock believes the chain is on one chain when it is on another one. Presuming this fork is long, the node ends up pruning beyond the fork point. And/or, the node ends up pruning out the alt chain that was deactivated. It feels like the caller (requester) should blow up at this point and either require manual assistance, or fall back to scanning over all unsafe tx’s, effectively “resetting” to the current chain tip.

TheBlueMatt commented at 5:56 pm on February 24, 2017:

True. I’m not sure what the right answer here is…clearly we dont want it to be so easy to miss transactions, but on the other hand, it sucks to not have the option here. A different approach might be to iterate over mapWallet and find all transactions which have a hash of a disconnected block, which I believe is correct as long as the block was, at some point, on the active chain, though might not be if the user is calling listsinceblock on a block that was never the active chain.

kallewoof commented at 6:14 pm on February 24, 2017:

Hm.. It almost feels like I can just get the block headers by iterating altchainindex up to the fork point and put those in a set and find the removed transactions in the initial mapWallet iteration. That way there’s no need to read in the blocks at all and can get rid of listed as well. Will try that, thanks for the nudge!

TheBlueMatt commented at 7:17 pm on February 24, 2017:

Yea, I was heasitant to recommend doing it alone because I’m not confident about how safe it is to make sure the wallet’s data is always right, will have to think more and review once you have it coded :).

kallewoof commented at 7:32 pm on February 24, 2017:

It seems reasonable, but I’m a bit stuck on how to get the block hash for a tx that is not in the main chain. It seems like CMerkleTx’s blockHash uint256 is only set if in the main chain…

TheBlueMatt commented at 9:43 pm on February 24, 2017:

Oh, sorry, you’re right…hashBlock is always something in the main chain (so you would only be able to print conflicted transactions and not transactions which went back to 0conf).

kallewoof commented at 11:12 pm on February 24, 2017:

Options: ①. Try to respect the path taken which requires the blocks as removed transactions do not track the block they were in, meaning if a node prunes out the block, the operation fails. ②. Ignore the path and simply show all removed transactions between the fork point and the chain tip, which requires NO loading of blocks (faster), but means the caller will get irrelevant removed transactions in reorgs it was never aware of to begin with.

Pros and cons with ①:

Pro: Caller gets exactly what they need to update their view of the chain state, and is not confused by irrelevant stuff.
Con: Slower due to block read requirement.
Con: Fails if blocks involved in the reorg were pruned out.

Pros and cons with ②:

Pro: Faster due to no block read.
Pro: Works fine even if involved blocks were pruned.
Con: Does not distinguish between the various forks involved.

TheBlueMatt commented at 11:21 pm on February 24, 2017:

There is a second con to ② - it cannot list transactions which went from confirmed to 0-conf, as they now have a hashBlock of null, which I think is somewhat of a blocker to using that approach. Another option is to have an optional parameter (default to off) which allows you to get partial-removed-data.

kallewoof commented at 1:56 am on February 25, 2017:

A 0-conf tx will be in the mempool right? Presuming it is, it will in fact be in the transactions array (I think).

The allow partial parameter makes sense to me. I think I’ll do that for starters until we make a decision (which may be another PR).

TheBlueMatt commented at 2:16 am on February 25, 2017:

That isnt a guarantee, because mempool may be limited.

kallewoof commented at 2:15 pm on April 17, 2017:

@TheBlueMatt I took the liberty to remove he allow_partial flag as I’ve seen more support for always throwing than for having the flag and only throwing sometimes. The unsquashed tree has everything in it still so it’d be no effort to resurrect this, if you or someone felt strongly enough about it.

sipa commented at 11:03 pm on July 12, 2017:

I agree with (1), and not having allow_partial. We may want to document that this feature stops working with pruning.

TheBlueMatt commented at 5:03 pm on February 24, 2017: member

utACK ad57cefb6583ab669d8da21d0bf808e2d38ef04a +/- some slight API nitpicks.

kallewoof force-pushed on Feb 24, 2017

kallewoof commented at 2:55 am on February 25, 2017: member

@TheBlueMatt I didn’t think about that.

But would the wallet actually do that to its own transaction? Throw out of mempool I mean.

kallewoof commented at 2:56 am on February 25, 2017: member

s/wallet/node/

sipa commented at 2:58 am on February 25, 2017: member

The wallet doesn’t control the mempool, and the mempool behaves independently. Anything else would be a privacy leak, as the mempool is observable through the P2P network.

luke-jr commented at 3:40 am on February 25, 2017: member

IMO if the stale block is pruned, throwing an exception is the right thing to do. The caller should be able to figure out its own rewinding back to the last common block.

(P.S. Perhaps the tests here should cover the case where there are three potential-chains involved?)

in src/wallet/rpcwallet.cpp: in 82184f9ac7 outdated

1797+            if (!allow_partial)
1798+            {
1799+                throw JSONRPCError(RPC_INTERNAL_ERROR, "Can't read block from disk");
1800+            }
1801+        }
1802+        else for (const CTransactionRef& tx : block.vtx)

luke-jr commented at 3:43 am on February 25, 2017:

No else for please.

0} else {
1    for (const CTransactionRef& tx : block.vtx) {

kallewoof commented at 5:13 pm on February 25, 2017: member

Good point about testing 3 chains. Will add a test like that.

kallewoof commented at 6:06 pm on February 25, 2017: member

I just realized the split network feature in the QA framework only supports one split (2 chains) right now. I think I’ll make a separate PR to extend that functionality and add more tests for 3+ chains after this is merged.

kallewoof force-pushed on Feb 26, 2017

kallewoof commented at 1:10 am on February 26, 2017: member

Squashed commits. Unsquashed branch preserved at https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed for comparison.

MarcoFalke commented at 12:09 pm on February 26, 2017: member

I just realized the split network feature in the QA framework only supports one split (2 chains) right now. I think I’ll make a separate PR to extend that functionality and add more tests for 3+ chains after this is merged.

Yes, please. Also make sure the extended functionality does not depend on the number of nodes in regtest being exactly 4.

On Sun, Feb 26, 2017 at 2:10 AM, kallewoof notifications@github.com wrote:

Squashed commits. Unsquashed branch preserved at https://github.com/kallewoof/bitcoin/tree/listsinceblock- include-lost-txs-unsquashed for comparison.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9622#issuecomment-282524695, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv98uWx_PRJaQ16WpuPIQGTvhEo8Wks5rgNEBgaJpZM4Lr4lC .

kallewoof commented at 5:24 am on February 27, 2017: member

@MarcoFalke See #9872.

jtimon commented at 8:43 pm on February 27, 2017: contributor

Concept ACK. Agree with @luke-jr in throwing the exception.

kallewoof force-pushed on Feb 28, 2017

kallewoof force-pushed on Mar 22, 2017

kallewoof commented at 7:37 pm on March 22, 2017: member

Rebased.

in test/functional/listsinceblock.py:129 in 104095b7db outdated

128+        }
129+        utxoDicts = [{
130+            'txid': utxo['txid'],
131+            'vout': utxo['vout'],
132+        }]
133+        txid1 = self.nodes[1].sendrawtransaction(

jnewbery commented at 4:24 pm on April 12, 2017:

Is it possible to use the sendtoaddress RPC here instead of {create,sign,send}rawtransaction? The rawtransaction RPCs force you to explicitly do things like set change addresses and make assumptions about fee rates, that could break this test in future.

kallewoof commented at 2:10 am on April 13, 2017:

Unfortunately sendtoaddress doesn’t let me choose the UTXO so I don’t think I can use it here.

in test/functional/listsinceblock.py:151 in 104095b7db outdated

146+        print('txid2 =', txid2)
147+
148+        # generate on both sides
149+        lastblockhash = self.nodes[1].generate(3)[2]
150+        self.nodes[2].generate(4)
151+        print('lastblockhash=%s' % (lastblockhash))

jnewbery commented at 4:26 pm on April 12, 2017:

Please don’t use print() calls in test cases. Either remove the line entirely, or add a self.log.debug() call if you think this information will be useful for debugging in future.

There are several other instances of this below. I won’t add additional review comments.

kallewoof commented at 2:11 am on April 13, 2017:

Removed all print()s. They seemed vaguely useful but not enough to warrant keeping them around long term.

in test/functional/listsinceblock.py:162 in 104095b7db outdated

157+        # gettransaction should work for txid1
158+        self.nodes[0].gettransaction(txid1)
159+
160+        # listsinceblock(lastblockhash) should now include txid1, as seen from nodes[0]
161+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
162+        assert_equal(any(tx['txid'] == txid1 for tx in lsbres['removed']), True)

jnewbery commented at 4:29 pm on April 12, 2017:

nit: no need for an assert_equal(True, True) here. Instead:

0assert any(tx['txid'] == txid1 for tx in lsbres['removed']), "txid1 not found in listsinceblock removed list"

in test/functional/listsinceblock.py:169 in 104095b7db outdated

161+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
162+        assert_equal(any(tx['txid'] == txid1 for tx in lsbres['removed']), True)
163+
164+        # but it should not include 'removed' if include_removed=false
165+        lsbres2 = self.nodes[0].listsinceblock(lastblockhash, 1, False, False)
166+        assert_equal('removed' in lsbres2, False)

jnewbery commented at 4:31 pm on April 12, 2017:

again, no need to assert_equal(False, False). Instead:

0assert 'removed' not in lsbres2

In general, we use assert_equal when comparing two variables because it prints the values of those variables, ie assert_equal(x, y) will print the values of x and y if they’re not equal, but assert x == y will not print the values of x and y. If you’re asserting the truthiness of one variable, you can just assert on that explicitly.

kallewoof commented at 2:24 pm on April 17, 2017:

I somehow skipped over this one. Fixed in squashed, unsquashed commit is https://github.com/kallewoof/bitcoin/commit/72ec1eaeda81c63a3e22fe0985ec02537082c9eb

in test/functional/listsinceblock.py:185 in 104095b7db outdated

186+
187+        1. tx1 is listed in listsinceblock.
188+        2. It is not included in 'removed' because it was not removed.
189+        3. It is listed with a confirmations count of 2 (bb3, bb4), not
190+           3 (aa1, aa2, aa3).
191+        '''

jnewbery commented at 4:32 pm on April 12, 2017:

Great docstrings :+1:

in test/functional/listsinceblock.py:196 in 104095b7db outdated

188+        2. It is not included in 'removed' because it was not removed.
189+        3. It is listed with a confirmations count of 2 (bb3, bb4), not
190+           3 (aa1, aa2, aa3).
191+        '''
192+
193+        assert_equal(self.is_network_split, False)

jnewbery commented at 4:33 pm on April 12, 2017:

as above for assert_equal(False, False). There are more instances of this below. I won’t add additional comments.

in test/functional/listsinceblock.py:161 in 104095b7db outdated

153+        self.sync_all()
154+
155+        self.join_network()
156+
157+        # gettransaction should work for txid1
158+        self.nodes[0].gettransaction(txid1)

jnewbery commented at 4:36 pm on April 12, 2017:

If you’re expecting this to work, can you assert on the returned value?

kallewoof commented at 2:23 am on April 13, 2017:

If the transaction cannot be gotten, an exception is raised (I swapped the txid out for a random hex below):

 02017-04-13 02:22:05.191000 TestFramework (ERROR): JSONRPC error
 1Traceback (most recent call last):
 2  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/test_framework.py", line 148, in main
 3    self.run_test()
 4  File "./listsinceblock.py", line 250, in run_test
 5    self.test_double_spend()
 6  File "./listsinceblock.py", line 155, in test_double_spend
 7    self.nodes[0].gettransaction("000000000009f75870f0ebe6f317af6928e3a74c676a60a0f11d3eab51c88ff3")
 8  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/coverage.py", line 46, in __call__
 9    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
10  File "/Users/karljohan-alm/workspace/bitcoin-kw/test/functional/test_framework/authproxy.py", line 153, in __call__
11    raise JSONRPCException(response['error'])
12test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)
132017-04-13 02:22:05.193000 TestFramework (INFO): Stopping nodes
142017-04-13 02:22:14.337000 TestFramework (WARNING): Not cleaning up dir /var/folders/b8/znqr8hj918772gfmd875gzgdd3ypz1/T/test5fli8q5i/13217
152017-04-13 02:22:14.337000 TestFramework (ERROR): Test failed. Test logging available at /var/folders/b8/znqr8hj918772gfmd875gzgdd3ypz1/T/test5fli8q5i/13217/test_framework.log

Asserting on the returned value is thus not necessary but I’m adding a comment indicating this.

jnewbery commented at 2:26 pm on April 20, 2017:

This comment is fine. Alternatively you could assert something like:

0assert self.nodes[0].gettransaction(txid1)['txid'] == txid1, "gettransaction failed to find txid1 not found"

kallewoof commented at 0:57 am on April 21, 2017:

Ohh, I didn’t realize assert also captured exceptions. Thanks, changed.

in test/functional/listsinceblock.py:253 in 104095b7db outdated

248+        # find transaction and ensure confirmations is valid
249+        for tx in lsbres['transactions']:
250+            if tx['txid'] == txid1:
251+                assert_equal(tx['confirmations'], 2)
252+
253+    def run_test(self):

jnewbery commented at 4:38 pm on April 12, 2017:

nit: I’d prefer run_test() to appear at the top of the test case, with the sub-tests below. When I come to read a test-case, it flows better for me if run test (which is called first) appears at the top.

kallewoof commented at 2:27 am on April 13, 2017:

Good idea. Changing.

in src/wallet/rpcwallet.cpp:1803 in 104095b7db outdated

1847+    UniValue removed(UniValue::VARR);
1848+    bool partial = false;
1849+    while (include_removed && paltindex && paltindex != pindex)
1850+    {
1851+        CBlock block;
1852+        if (!ReadBlockFromDisk(block, paltindex, Params().GetConsensus()))

jnewbery commented at 4:42 pm on April 12, 2017:

nit: it’d be nice to test this branch in the functional test, both with and without the partial flag.

in src/wallet/rpcwallet.cpp:1439 in 104095b7db outdated

1409+ * @param  nMinDepth  The minimum confirmation depth.
1410+ * @param  fLong      Whether to include the JSON version of the transaction.
1411+ * @param  ret        The UniValue into which the result is stored.
1412+ * @param  filter     The "is mine" filter bool.
1413+ * @return            true if appends were made to ret, false otherwise.
1414+ */

jnewbery commented at 4:44 pm on April 12, 2017:

:+1: comments!

jnewbery commented at 5:28 pm on April 12, 2017: member

I’ve only reviewed the test code. It’s a shame that this makes the listsinceblock test runtime increase (from 32s to 72s on recent travis runs), although #10198 and #10082 will help with that.

One design comment: I’m not convinced it’s a good idea to not including duplicate transactions in the removed array. I think that could be a trap for users. My expectation would be that I could periodically run this RPC to get the list of transactions since last time I ran the command (by passing in the lastblock value from the previous call). I’d expect to be able to keep a set of transactions, adding the transactions array to my set and subtracting the removed array. If you don’t include the duplicate transactions in the removed array, then the user might end up with more than one instance of a transaction in their set.

Obviously the user should look out for duplicates and not add them to their set, but if they’re doing something daft like keying off the blockhash and txid, then they’ll get into this inconsistent state.

I don’t think that’s enough to block this PR, but I think it’s considering.

kallewoof force-pushed on Apr 13, 2017

kallewoof commented at 2:49 am on April 13, 2017: member

@jnewbery Thanks a lot for the review! I’ve addressed most of your concerns, except for the nit about testing allow_partial flag.

Regarding duplicates in removed, I’m neutral personally. Allowing duplicates decreases the code size, so I’m all for the switch personally. (Clarification: I changed the code to allow duplicates.)

Unsquashed (https://github.com/kallewoof/bitcoin/commit/8871340d2aeb5e2e7888430eee0ed358c67c3030 is previous head): https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed

https://github.com/kallewoof/bitcoin/commit/c7adf4aee94c5663826af4b277bca632e4f5cf31 addresses nits
https://github.com/kallewoof/bitcoin/commit/12831108c0abdc088814a3c3d65c5e75387f05b0 and https://github.com/kallewoof/bitcoin/commit/2475789299304366c86f790fb05de27edea679b6 (test tweak) fixes duplicates; if people oppose this change I can drop these commits easily enough

kallewoof force-pushed on Apr 13, 2017

luke-jr commented at 1:41 pm on April 17, 2017: member

Do you plan to remove the allow_partial so it always throws an exception?

kallewoof commented at 1:59 pm on April 17, 2017: member

@luke-jr I would love to, personally. I wasn’t sure if it was the agreed approach, in the end, but more people have agreed about throwing than not, so I’m going to remove it for now.

Edit: I removed the allow_partial functionality. (The unsquashed results are in the same spot (https://github.com/kallewoof/bitcoin/tree/listsinceblock-include-lost-txs-unsquashed) but since the end result was dropping of commits with some conflict fixes the unsquashed tree still has the allow_partial related commits.)

kallewoof force-pushed on Apr 17, 2017

jnewbery commented at 2:24 pm on April 17, 2017: member

Thanks @kallewoof - I don’t really mind too much either way. If I were coding this from scratch, I would prefer to show duplicates in removed, but I’m happy to go along with the consensus and since you’ve already put in the code to hide duplicates, that’s also fine.

kallewoof commented at 2:40 pm on April 17, 2017: member

@jnewbery Duplicates are shown in removed now. What I removed was a different feature, where a user could pass in an ‘allow partial’ flag which would prevent the node from throwing an exception if a block couldn’t be read (useful for pruned nodes), but several people preferred always throwing.

kallewoof commented at 3:35 am on April 20, 2017: member

Realized I had missed one assert_equal(x, false) -> assert not x.

Unsquashed history:

Mar 23: 1 → 2 → 3⊱1 → ~~4⊱1~~ → ~~5⊱1~~ → 6⊱1 → 7⊱1
Apr 13: → 8⊱2 → 9⊱1 → 10⊱2 → 11⊱1
Apr 17: → 12⊱2
Apr 20: → 13⊱2

kallewoof force-pushed on Apr 20, 2017

in src/wallet/rpcwallet.cpp:1813 in a8c56bf7b7 outdated

1809@@ -1792,6 +1810,8 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1810         filter = filter | ISMINE_WATCH_ONLY;
1811     }
1812 
1813+    bool include_removed = (request.params.size() < 4 || request.params[3].get_bool());

jnewbery commented at 2:07 pm on April 20, 2017:

When adding RPCs or arguments, good advice is to treat null arguments the same as missing arguments: #10143 (review)

I think that means this line would become:

bool include_removed = (request.params.size() < 4 || request.params[3].isNull() || request.params[3].get_bool());

in test/functional/listsinceblock.py:21 in a8c56bf7b7 outdated

13@@ -14,7 +14,12 @@ def __init__(self):
14         self.setup_clean_chain = True
15         self.num_nodes = 4
16 
17-    def run_test (self):
18+    def run_test(self):
19+        self.test_reorg()

jnewbery commented at 2:37 pm on April 20, 2017:

Now that you’ve split this into sub-tests, can you move the lines:

0self.nodes[2].generate(101)
1self.sync_all()

into the run_test() function before calling test_reorg(). It’s best if sub-tests have no shared state between them. You should be able to re-order or skip tests without breaking them, and generating the chain in test_reorg() breaks that assumption.

We want sub-tests to be as independent as possible so if they break it’s easier to debug where the problem is.

kallewoof commented at 0:54 am on April 21, 2017:

Done - but some tweaking is necessary to make them truly independent of order (e.g. instead of asserting on getbalance()s, should just grab balance and compare). Will not do that work in this PR, but will try to address it asap unless someone else gets to it before me.

in test/functional/listsinceblock.py:147 in a8c56bf7b7 outdated

143+        # send from nodes[2] using utxo to nodes[3]
144+        recipientDict2 = {
145+            self.nodes[3].getnewaddress(): 1,
146+            self.nodes[2].getnewaddress(): change,
147+        }
148+        txid2 = self.nodes[2].sendrawtransaction(

jnewbery commented at 2:39 pm on April 20, 2017:

txid2 is unused so you don’t need to assign it here.

in test/functional/listsinceblock.py:168 in a8c56bf7b7 outdated

164+        # listsinceblock(lastblockhash) should now include txid1, as seen from nodes[0]
165+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
166+        assert any(tx['txid'] == txid1 for tx in lsbres['removed'])
167+
168+        # but it should not include 'removed' if include_removed=false
169+        lsbres2 = self.nodes[0].listsinceblock(lastblockhash, 1, False, False)

jnewbery commented at 2:41 pm on April 20, 2017:

consider using named arguments here so it’s clear to the reader what the arguments are for (you can also omit the optional target_confirmations and include_watchonly arguments)

in src/wallet/rpcwallet.cpp:1776 in a8c56bf7b7 outdated

1772@@ -1756,7 +1773,8 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1773 
1774     LOCK2(cs_main, pwallet->cs_wallet);
1775 
1776-    const CBlockIndex *pindex = NULL;
1777+    const CBlockIndex* pindex = NULL;

jnewbery commented at 2:48 pm on April 20, 2017:

These variables might warrant their own comments now, since it’s not immediately obvious what they’re doing:

pindex - transactions from this block onwards should be included in the result. If the specified block was not in the main chain, pindex is the block where the chain forked paltindex - used to count back from the specified block to the fork point to collect transactions for the removed array.

You can probably come up with better wording.

jnewbery approved

jnewbery commented at 2:49 pm on April 20, 2017: member

Looks good. A few nits but otherwise ACK https://github.com/bitcoin/bitcoin/pull/9622/commits/a8c56bf7b765910fb049373655c2823c7f8b6971

kallewoof force-pushed on Apr 21, 2017

kallewoof commented at 1:27 am on April 21, 2017: member

@jnewbery Thanks for feedback! I think I addressed all of the stuff you mentioned. Compare 14⊱1 & 15⊱2.

[…]:

Apr 20: → 13⊱2
Apr 21: → 14⊱1 → 15⊱2

jnewbery commented at 7:00 pm on April 28, 2017: member

Looks good. ReACK https://github.com/bitcoin/bitcoin/pull/9622/commits/4578a21bc29768e11f56be600b141d81447db7a6

kallewoof force-pushed on May 8, 2017

kallewoof commented at 4:45 am on May 8, 2017: member

Rebased due to conflicts.

Edit: removed some irrelevant asserts that started causing errors in tests. (Not sure why a clean chain with generate(101) would yield 5100 BTC instead of 50, but no matter.)

kallewoof force-pushed on May 8, 2017

jnewbery commented at 6:18 pm on May 15, 2017: member

Edit: removed some irrelevant asserts that started causing errors in tests. (Not sure why a clean chain with generate(101) would yield 5100 BTC instead of 50, but no matter.)

Looks like you’ve reintroduced the lines:

0        self.nodes[2].generate(101)
1        self.sync_all()

into the test_reorg() function, which means you’re now generating 202 blocks (so node 2 now has 102 spendable coinbases = 5100 BTC).

I think this is just a bad rebase.

kallewoof commented at 0:42 am on May 16, 2017: member

@jnewbery Ahh, thanks. Yeah, that is obviously it. I am just going to remove the double generate in test_reorg.

[…]:

May 16: → 16⊱2

kallewoof force-pushed on May 16, 2017

luke-jr referenced this in commit 1405b803b5 on Jun 15, 2017

luke-jr referenced this in commit c50e5b77f1 on Jun 15, 2017

luke-jr referenced this in commit bd73314fe8 on Jun 18, 2017

in src/wallet/rpcwallet.cpp:1752 in fb17746c45 outdated

1749+    const CBlockIndex* paltindex = NULL; // Block index of the specified block, even if it's in a deactivated chain.
1750     int target_confirms = 1;
1751     isminefilter filter = ISMINE_SPENDABLE;
1752 
1753-    if (request.params.size() > 0)
1754+    if (request.params.size() > 0 && !request.params[0].isNull())

sipa commented at 10:57 pm on July 12, 2017:

Nit: the request.params.size() > 0 check is superfluous with this, as the operator[] will always return a null UniValue if out of range. (and several other places)

kallewoof commented at 1:25 am on July 13, 2017:

Oh, I didn’t know that. That makes things cleaner, thanks.

in src/wallet/rpcwallet.cpp:1800 in fb17746c45 outdated

1793@@ -1774,11 +1794,35 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1794             ListTransactions(pwallet, tx, "*", 0, true, transactions, filter);
1795     }
1796 
1797+    // when a reorg'd block is requested, we also list any relevant transactions
1798+    // in the blocks of the chain that was detached
1799+    UniValue removed(UniValue::VARR);
1800+    while (include_removed && paltindex && paltindex != pindex)

sipa commented at 10:57 pm on July 12, 2017:

Nit: braces on the same line (and many other places).

sipa commented at 11:05 pm on July 12, 2017: member

utACK 5d352044ca7f29f992b4de46d461bccc86326ba1 with a few nits. I did not review the tests.

kallewoof force-pushed on Jul 13, 2017

kallewoof commented at 1:34 am on July 13, 2017: member

@sipa Thanks for the review. I’ve addressed your nits in https://github.com/kallewoof/bitcoin/commit/3a10e7c49108dde97d999f46bbb36ce905887a0e

in src/wallet/rpcwallet.cpp:1853 in 0d457c7b27 outdated

1854+        for (const CTransactionRef& tx : block.vtx) {
1855+            if (pwallet->mapWallet.count(tx->GetHash()) > 0) {
1856+                // Use -depth as minDepth parameter to ListTransactions to prevent incoming
1857+                // transactions from being filtered. These transactions have negative
1858+                // confirmations, but always greater than -depth.
1859+                ListTransactions(pwallet, pwallet->mapWallet[tx->GetHash()], "*", -depth, true, removed, filter);

TheBlueMatt commented at 6:31 pm on July 14, 2017:

Hmm, technically -depth isnt sufficient here. In the rare case of a reorg-to-lower-block-height this would be insufficient. You already got the block from the chain, just call it - 1000000000 or so.

in src/wallet/rpcwallet.cpp:1793 in 0d457c7b27 outdated

1775@@ -1761,7 +1776,10 @@ UniValue listsinceblock(const JSONRPCRequest& request)
1776             "    \"comment\": \"...\",       (string) If a comment is associated with the transaction.\n"
1777             "    \"label\" : \"label\"       (string) A comment for the address/transaction, if any\n"
1778             "    \"to\": \"...\",            (string) If a comment to is associated with the transaction.\n"
1779-             "  ],\n"
1780+            "  ],\n"
1781+            "  \"removed\": [\n"

TheBlueMatt commented at 6:32 pm on July 14, 2017:

Can we add something to note that transactions which were re-added are included here anyway, and may have, at that point, positive confirmations value in this array?

in test/functional/listsinceblock.py:181 in 0d457c7b27 outdated

177+                    bb4
178+
179+        Asserted:
180+
181+        1. tx1 is listed in listsinceblock.
182+        2. It is not included in 'removed' because it was not removed.

TheBlueMatt commented at 6:34 pm on July 14, 2017:

This is wrong (and the check and later comment contradict this).

in test/functional/listsinceblock.py:239 in 0d457c7b27 outdated

234+        lsbres = self.nodes[0].listsinceblock(lastblockhash)
235+        assert any(tx['txid'] == txid1 for tx in lsbres['transactions'])
236+        assert any(tx['txid'] == txid1 for tx in lsbres['removed'])
237+
238+        # find transaction and ensure confirmations is valid
239+        for tx in lsbres['transactions']:

TheBlueMatt commented at 6:34 pm on July 14, 2017:

Can you duplicate this loop for “removed”, noting that the tx listed in “removed” should also have a confirmations of 2.

kallewoof force-pushed on Jul 18, 2017

kallewoof commented at 4:33 am on July 18, 2017: member

@TheBlueMatt Thanks for the review! I’ve addressed all of your nits, I believe. The unsquashed changes are in https://github.com/kallewoof/bitcoin/commit/5166d0e4f819ab4b430103fa350f8f882f1b1e2f and https://github.com/kallewoof/bitcoin/commit/299c00c56b4361c46a2dd9cc2fa4d4beb8bc2250.

TheBlueMatt commented at 9:36 pm on July 18, 2017: member

I think this maybe missed 15, sadly. Its a nice change, but not a bugfix.

kallewoof commented at 2:12 am on July 19, 2017: member

That’s disheartening, but ah well.

TheBlueMatt commented at 8:42 pm on July 19, 2017: member

utACK d6115c21a790e139bb45ec6dee421a976bc21eaf. Yea, Core has been in a near-constant state of growing pains for some time it seems. Recently its grown active enough to be just beyond the ability of any individual to keep up with everything, and so things occasionally move slower than they should :(. Anyway, up to @laanwj, this isnt a bugfix, but its pretty clean so maybe he’s still willing to pull it for 15.

kallewoof commented at 11:12 pm on July 19, 2017: member

I’d say let’s just push it to 16 milestone. I’ll be noisy about it this time. :)

TheBlueMatt commented at 11:18 pm on July 19, 2017: member

Heh, yes, if things have been sitting for a while definitely be noisy!

On July 19, 2017 7:12:08 PM EDT, kallewoof notifications@github.com wrote:

I’d say let’s just push it to 16 milestone. I’ll be noisy about it this time. :)

laanwj commented at 3:06 pm on July 20, 2017: member

Needs rebase.

I’d say let’s just push it to 16 milestone. I’ll be noisy about it this time. :)

Up to you, though given how long this has been open and how much interest and review this has I don’t have a particular problem with merging this into 0.15 still.

listsinceblock: optionally find and list any transactions that were undone due to reorg when requesting a non-main chain block in a new 'removed' array. f999c46cae

Testing: listsinceblock should display all transactions that were affected since the given block, including transactions that were removed due to a reorg. 876e92bf54

kallewoof force-pushed on Jul 21, 2017

kallewoof commented at 2:01 am on July 21, 2017: member

@laanwj Rebased. And I’d love to have this in 15, personally. I just don’t wanna rush anything for the sake of the PR being old.

sipa commented at 5:11 pm on July 21, 2017: member

utACK

laanwj merged this on Jul 24, 2017

laanwj closed this on Jul 24, 2017

laanwj referenced this in commit 6ef3c7ec62 on Jul 24, 2017

kallewoof deleted the branch on Jul 24, 2017

PastaPastaPasta referenced this in commit 69ddb22ee7 on Aug 6, 2019

PastaPastaPasta referenced this in commit 0f9878f438 on Aug 6, 2019

PastaPastaPasta referenced this in commit 747085410c on Aug 6, 2019

PastaPastaPasta referenced this in commit a026b74f9e on Aug 7, 2019

PastaPastaPasta referenced this in commit 6ccec5915f on Aug 8, 2019

PastaPastaPasta referenced this in commit f393729498 on Aug 12, 2019

barrystyle referenced this in commit a23d53ae0a on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

[rpc] listsinceblock should include lost transactions when parameter is a reorg’d block #9622