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

In listaddressgroupings push down the IsMine check to run on each input. #1872

Merged
merged 1 commit into from
Jan 22, 2013
Merged

In listaddressgroupings push down the IsMine check to run on each input. #1872

merged 1 commit into from
Jan 22, 2013

Conversation

gmaxwell
Copy link
Contributor

This avoids a potential crash when trying to read the scrippubkeys on
transactions where the first input IsMine but some of the rest are not
when running listaddressgroupings.

@BitcoinPullTester
Copy link

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

@sipa
Copy link
Member

sipa commented Sep 28, 2012

Haven't tested, but code changes look ok.

@gavinandresen
Copy link
Contributor

Sanity testing on a testnet-in-a-box "hell wallet", I get an extra empty array:

Old:

gavin$ head /tmp/f2
[
    [
        [
            "mmU33ZKwUkSoVSx8bHUnf2PmNosDMWvxsU",
            17.93950000
        ],
        [
            "mzGzLeJA2up3VL4gW32x5hT5YvEaA7SnBh",
            0.00000000
        ],

New:

gavin$ head /tmp/f1
[
    [
    ],
    [
        [
            "mmU33ZKwUkSoVSx8bHUnf2PmNosDMWvxsU",
            17.93950000
        ],
        [
            "mzGzLeJA2up3VL4gW32x5hT5YvEaA7SnBh",

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Oct 2, 2012

@gavinandresen Ah. Thanks. I'm pretty sure thats just from adding an empty group after encountering a transaction where it can't add any linked ins or outs. Pushing a revised patch that I expect will fix that (though I don't have a reproduction setup right now). It might be helpful to encrypt that wallet with some impossible key and post it.

@gavinandresen
Copy link
Contributor

It is a testnet-in-a-box wallet, so it is not sensitive.

Wallet and blockchain are at:
http://www.skypaint.com/bitcoin/gavin_testnetbox_wallet.tar.gz

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Oct 4, 2012

Indeed, the empty group is gone with the latest update.

@jgarzik
Copy link
Contributor

jgarzik commented Oct 9, 2012

ACK

@BitcoinPullTester
Copy link

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

BOOST_FOREACH(CTxOut txout, pcoin->vout)
if (IsChange(txout))
{
CWalletTx tx = mapWallet[pcoin->vin[0].prevout.hash];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to refer to vin[0] specifically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even better, what is that 'tx' variable doing there at all?

@gavinandresen
Copy link
Contributor

ACK if you remove the CWalletTx tx = .... dead line of code that sipa pointed out.

This avoids a potential crash when trying to read the scrippubkeys on
transactions where the first input IsMine but some of the rest are not
when running listaddressgroupings.
@gmaxwell
Copy link
Contributor Author

Yea, dunno how that made it into there.. cruft from an earlier version. Sorry about that. Fixed and tested.

@BitcoinPullTester
Copy link

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

@Diapolo
Copy link

Diapolo commented Jan 11, 2013

This one is lingering around for quite some time, I count 2 ACKs, any reason not to merge it or does it need a rebase?

gmaxwell added a commit that referenced this pull request Jan 22, 2013
In listaddressgroupings push down the IsMine check to run on each input.
@gmaxwell gmaxwell merged commit d40c164 into bitcoin:master Jan 22, 2013
laudney pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request Mar 19, 2014
In listaddressgroupings push down the IsMine check to run on each input.
KolbyML pushed a commit to KolbyML/bitcoin that referenced this pull request Dec 5, 2020
…killed main.{h,cpp})

34ad9d5 [Cleanup] Fix cs_main in net_processing (random-zebra)
f798b5f Rename the remaining main.{h,cpp} to validation.{h,cpp} (random-zebra)
be294cb [Validation] Move Block spam filter to net_processing (random-zebra)
520ae4b Move network-msg-processing code out of main to its own file (random-zebra)
9b894ed [Validation] save mapBlockSource in ProcessMessage (not ProcessNewBlock) (random-zebra)
2f37add Remove orphan state wipe from UnloadBlockIndex. (Matt Corallo)

Pull request description:

  Finally split main.{h,cpp} file in net_processing.{h,cpp}/validation.{h,cpp} as per upstream (bitcoin#9260) 🎉

  Built on top of
  - [x] bitcoin#1866

ACKs for top commit:
  Fuzzbawls:
    ACK 34ad9d5
  furszy:
    ACK 34ad9d5 with a minor nit.

Tree-SHA512: 18b1457759d8684344c271236c63e1d9c83ed6e651da3efd2a7d8ab5af577fdfa85fd0f08107a26f952c3ae5da0793ebb77a2c5a710c65161f5132178db931e3
jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Jul 18, 2021
This commit fixes a crash from an out-of-range vector element access attempt
that stems from the same situation with Gridcoin as in the following Bitcoin
PR: bitcoin/bitcoin#1872.

Very strange our code did not already have this change.
jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Jul 18, 2021
This commit fixes a crash from an out-of-range vector element access attempt
that stems from the same situation with Gridcoin as in the following Bitcoin
PR: bitcoin/bitcoin#1872.

Very strange our code did not already have this change.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants