Fixes for when listtransactions encounters non-standard outputs #1850

luke-jr · 2012-09-22T03:27:13Z

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.

luke-jr · 2012-09-22T03:28:19Z

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 · 2012-09-23T21:58:38Z

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

gavinandresen · 2012-10-01T21:05:30Z

src/rpcwallet.cpp

+static void MaybePushAddress(Object & entry, const CTxDestination &dest)
+{
+    CBitcoinAddress addr;
+    if (!addr.Set(dest))


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

gavinandresen · 2012-10-01T21:11:00Z

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 · 2012-10-20T21:59:58Z

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

sipa · 2012-10-25T18:40:18Z

src/wallet.cpp

        if (nDebit > 0)
            listSent.push_back(make_pair(address, txout.nValue));

-        if (pwallet->IsMine(txout))
+        if (fIsMine)


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

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

luke-jr · 2012-11-15T00:41:25Z

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

I'll get to tests later.

BitcoinPullTester · 2012-11-24T00:28:33Z

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

sipa · 2012-11-27T21:09:15Z

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

gavinandresen · 2013-01-23T15:16:33Z

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

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

…ation isn't recognized Previously, it would pass corrupt/random through base58.

luke-jr · 2013-07-17T03:08:30Z

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 · 2013-07-20T12:11:26Z

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 · 2013-09-19T07:56:41Z

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 · 2013-10-07T11:57:44Z

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

Diapolo · 2013-10-07T12:11:02Z

src/wallet.cpp

        CTxDestination address;
-        vector<unsigned char> vchPubKey;
        if (!ExtractDestination(txout.scriptPubKey, address))
        {
            printf("CWalletTx::GetAmounts: Unknown transaction type found, txid %s\n",


This creates a merge-conflict ^^.

Diapolo · 2013-10-10T12:50:17Z

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

gavinandresen · 2013-10-22T01:43:23Z

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 · 2013-10-22T05:36:25Z

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

…oins selection flow removed. 5935497 Making CPivStake immutable. (furszy) af1bf37 wallet::CreateCoinStake set missing pindeFrom in stakeInput. (furszy) f935801 wallet: Implement a slightly different StakeableCoins, initializing and returning CStakeableOutput. (furszy) bbc7d77 wallet: CheckTXAvailability returning the block index if is requested. (furszy) d8dbf49 CPivStake: Introducing SetIndexFrom method. (furszy) 9a5c090 wallet: Introducing StakeableOutput class. (furszy) f9edbd5 wallet: AvailableCoins using the new CheckOutputAvailability method. (furszy) 7212f79 Wallet: new CheckOutputAvailability method implemented. Decoupling and cleaning the output availability checks from AvailableCoins (furszy) 1128da0 CPivStake: make pindexFrom member const (furszy) 3c27c3d CPivStake: removing unreachable code from GetIndexFrom(). (furszy) a5c759c CPivStake: store only the needed CTxOut and COutPoint data and not the entire transaction. (furszy) 0cd01a4 CStakeInput: remove unused GetTxFrom method. (furszy) Pull request description: Essentially, this work is removing an unneeded `GetTransaction` call from `CPivStake::GetIndexFrom` for every stakeable output during each cycle of the staking process. As `GetTransaction` is a direct access to disk (txindex), this means faster staking. Future work, the methods `StakeableCoins` and `AvailableCoins` can be abstracted even more. They are sharing almost the same code with an specific distinction that makes this change possible without introducing an extra blockIndex set into `AvailableCoins`. ACKs for top commit: random-zebra: But these are just styling changes, so if you're not happy making them, I'm fine with the PR as is. Tested ACK 5935497 Fuzzbawls: ACK 5935497 Tree-SHA512: cc34443c16f678c51529a5e3cf682ab5c8edadb9c0c307130a93f9f5d1cc0707121cbb278c86197d595c679e479b7850c21180334a196d7e72d064feac267525

gavinandresen reviewed Oct 1, 2012
View reviewed changes

sipa reviewed Oct 25, 2012
View reviewed changes

luke-jr added 2 commits July 17, 2013 03:03

Bugfix: Avoid trying to parse outputs that aren't relevant to CWallet…

96ed682

…Tx::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 destin…

cc6cfab

…ation isn't recognized Previously, it would pass corrupt/random through base58.

luke-jr mentioned this pull request Aug 2, 2013

Payment Protocol Work #2539

Merged

luke-jr mentioned this pull request Sep 19, 2013

Using payment-request adds strange address to address book #3006

Closed

Diapolo reviewed Oct 7, 2013
View reviewed changes

gavinandresen merged commit cc6cfab into bitcoin:master Oct 22, 2013

luke-jr deleted the bugfix_unknownoutputs branch October 19, 2014 22:19

hrobeers mentioned this pull request Nov 23, 2017

Fixes for when listtransactions encounters non-standard outputs peercoin/peercoin#247

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fixes for when listtransactions encounters non-standard outputs #1850

Fixes for when listtransactions encounters non-standard outputs #1850

Uh oh!

Conversation

luke-jr commented Sep 22, 2012

Uh oh!

luke-jr commented Sep 22, 2012

Uh oh!

BitcoinPullTester commented Sep 23, 2012

Uh oh!

gavinandresen Oct 1, 2012

Choose a reason for hiding this comment

Uh oh!

gavinandresen commented Oct 1, 2012

Uh oh!

sipa commented Oct 20, 2012

Uh oh!

sipa Oct 25, 2012

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 15, 2012

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Nov 15, 2012

Uh oh!

BitcoinPullTester commented Nov 24, 2012

Uh oh!

sipa commented Nov 27, 2012

Uh oh!

gavinandresen commented Jan 23, 2013

Uh oh!

luke-jr commented Jul 17, 2013

Uh oh!

BitcoinPullTester commented Jul 20, 2013

Uh oh!

Diapolo commented Sep 19, 2013

Uh oh!

Diapolo commented Oct 7, 2013

Uh oh!

Diapolo Oct 7, 2013

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Oct 10, 2013

Uh oh!

gavinandresen commented Oct 22, 2013

Uh oh!

gavinandresen commented Oct 22, 2013

Uh oh!

Uh oh!