Fixes for when listtransactions encounters non-standard outputs

luke-jr commented at 3:27 am on September 22, 2012: member

Bugfix: Avoid trying to parse outputs that aren’t relevant to CWalletTx::GetAmounts

This fixes a warning when an output we aren’t concerned with can’t be parsed.

Bugfix: Supress “address” key in transaction details, when the destination isn’t recognized

Previously, it would pass corrupt/random through base58.

Fixes #779 and #1848.

luke-jr commented at 3:28 am on September 22, 2012: member

Needs to be addressed: when the address is unavailable, should the “address” key be absent, null, or something else? Current code here does the “absent” option.

BitcoinPullTester commented at 9:58 pm on September 23, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/1a5eb4636d038fdf623924c0d236c915ddcb9bdb for binaries and test log.

in src/rpcwallet.cpp: in 1a5eb4636d outdated

903@@ -904,6 +904,14 @@ Value listreceivedbyaccount(const Array& params, bool fHelp)
904     return ListReceived(params, true);
905 }
906 
907+static void MaybePushAddress(Object & entry, const CTxDestination &dest)
908+{
909+    CBitcoinAddress addr;
910+    if (!addr.Set(dest))

gavinandresen commented at 9:05 pm on October 1, 2012:

Nit: if (addr.Set(dest)) entry.push_back() would be clearer logic.

gavinandresen commented at 9:11 pm on October 1, 2012: contributor

This is high risk for breaking existing applications; in addition to careful code review I would really like there to be a test plan, or, even better, until tests that fully exercise all of the code paths in CWalletTx::GetAmounts.

sipa commented at 9:59 pm on October 20, 2012: member

I hope no application depends on the random data being produced, so I think this should be treated as a bugfix.

in src/wallet.cpp: in 1a5eb4636d outdated

652-
653         if (nDebit > 0)
654             listSent.push_back(make_pair(address, txout.nValue));
655 
656-        if (pwallet->IsMine(txout))
657+        if (fIsMine)

sipa commented at 6:40 pm on October 25, 2012:

This if is redundant, if fIsMine is false, we already bailed out.

luke-jr commented at 0:40 am on November 15, 2012:

It shouldn’t be, thanks for noticing the bug :)

luke-jr commented at 0:41 am on November 15, 2012: member

Fixed the bug @sipa found, and commented CWalletTx::GetAmounts better so the flow is more understandable.

I’ll get to tests later.

BitcoinPullTester commented at 0:28 am on November 24, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d2233dbcf4bf520370ffbd59b1a42718e4f68d27 for binaries and test log.

sipa commented at 9:09 pm on November 27, 2012: member

Looks like just code movement to me (except the address = CNoDestination()), ACK. I haven’t tested though.

gavinandresen commented at 3:16 pm on January 23, 2013: contributor

Needs a test plan; see https://github.com/bitcoin/QA for a test plan skeleton.

Bugfix: Avoid trying to parse outputs that aren't relevant to CWalletTx::GetAmounts

This fixes a warning when an output we aren't concerned with can't be parsed.

96ed682176

Bugfix: Supress "address" key in transaction details, when the destination isn't recognized

Previously, it would pass corrupt/random through base58.

cc6cfab38f

luke-jr commented at 3:08 am on July 17, 2013: member

Fixed @gavinandresen ’s nit and a bug I thought @jgarzik reported with it (though it seems to have vanished here…), and rebased.

I am not sure how to create a test plan for these particular fixes. I believe the conditions to trigger the bug can only occur when some other (eg, newer) client has accepted wallet transactions that the currently running version does not understand.

BitcoinPullTester commented at 12:11 pm on July 20, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/cc6cfab38fcdd4ec3e4784e01c4407a129289f77 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.

Diapolo commented at 7:56 am on September 19, 2013: none

Is this the last version and rebased to current master? If yes I’m going to try if this fixes my problem in #3006.

Diapolo commented at 11:57 am on October 7, 2013: none

@luke-jr Can you please rebase this or is it mergable?

in src/wallet.cpp: in cc6cfab38f

690+        // In either case, we need to get the destination address
691         CTxDestination address;
692-        vector<unsigned char> vchPubKey;
693         if (!ExtractDestination(txout.scriptPubKey, address))
694         {
695             printf("CWalletTx::GetAmounts: Unknown transaction type found, txid %s\n",

Diapolo commented at 12:11 pm on October 7, 2013:

This creates a merge-conflict ^^.

Diapolo commented at 12:50 pm on October 10, 2013: none

@luke-jr Are you still around here or don’t you participate anymore?

gavinandresen commented at 1:43 am on October 22, 2013: contributor

This is on my list of old pulls to get resolved.

After reviewing the code again, I’m planning on:

Tweaking my handy-dandy payment request generator code so I have an easy way of generating transactions that pay to standard-but-not-expressible-as-an-address transactions (I’ll teach it to take hex full public keys OR addresses and have it do-the-right-thing; I’ve been meaning to do that for more payment protocol testing anyway).
Test this code. I waffle on whether it would be better to output “address” : null instead of leaving out “address” for outputs that cannot be expressed as an address. Certainly the current behavior is wrong…

gavinandresen merged this on Oct 22, 2013

gavinandresen closed this on Oct 22, 2013

gavinandresen commented at 5:36 am on October 22, 2013: contributor

Pulled (after testing using https://bitcoincore.org/~gavin/createpaymentrequest.php and fixing the trivial merge conflict).

ummjackson referenced this in commit f4ce7fcc2e on Feb 8, 2014

luke-jr deleted the branch on Oct 19, 2014

KolbyML referenced this in commit 9520ee370b on Dec 5, 2020

DrahtBot locked this on Sep 8, 2021

Fixes for when listtransactions encounters non-standard outputs #1850