Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Transactions attributed to wrong address under certain conditions #689

Closed
Plaidxx opened this Issue Dec 7, 2011 · 3 comments

Comments

Projects
None yet
2 participants

Plaidxx commented Dec 7, 2011

If you receive coins via two or more addresses in the same wallet, where the coins originate from the same sendmany transaction, the sum total of the coins will be attributed to the first receiving address and the others won't show up.

To reproduce:

  1. Set up testnet in a box
  2. Start bitcoin-qt pointing at datadir 1 (instance 1)
  3. Start bitcoin-qt pointing at datadir 2 (instance 2)
  4. In instance 2, create 2 new receiving addresses labeled Address A and Address B
  5. In instance 1, create a new transaction sending 1 BTC to Address A. Click "Add Recipient" and add a second payment sending 2 BTC to Address B. Send the payment.
  6. In instance 2, it will display that you received 3 BTC via Address A, and Address B will not show up anywhere.

Plaidxx commented Dec 10, 2011

I haven't had much time to look at the code, but it might be the case that CWallet::AddToWallet() is assuming that a given transaction will only ever have one payment / payment address in it (for this wallet). But when sendmany is used, any given transaction can have more than one payment to separate addresses in this wallet.

Owner

laanwj commented Dec 11, 2011

This is because �TransactionRecord::decomposeTransaction, where the bitcoin transactions are decomposed to UI transactions, handles the case where multiple outputs are "mine" by choosing one of the addresses.

            // Received by Bitcoin Address
            sub.type = TransactionRecord::RecvWithAddress;
            BOOST_FOREACH(const CTxOut& txout, wtx.vout)
            {
                if(wallet->IsMine(txout))
                {
                    CBitcoinAddress address;
                    if (ExtractAddress(txout.scriptPubKey, wallet, address))
                    {
                        sub.address = address.ToString();
                    }
                    break;
                }
            }

(I agree it would be better to generate multiple UI transactions in this case, one for each owned target address)

Owner

laanwj commented Feb 5, 2012

Can you test pull request #800?

@laanwj laanwj added a commit to laanwj/bitcoin that referenced this issue Feb 6, 2012

@laanwj laanwj Restructure credit transaction decomposition (solves issue #689)
When a transaction has multiple outputs that go to the wallet, list these
as multiple transactions in the UI. This is also applied to generated
(coinbase) transactions. Also makes the code shorter and easier
to understand.
ab07866

@Matoking Matoking added a commit to Matoking/bitcoin that referenced this issue Feb 8, 2012

@Matoking Matoking *Fix Minimize to the tray instead of the taskbar
Avoid advertising the node's address when it is not listening or IsInitialBlockDownload().

This also avoids flushing setAddrKnown until 24 hours has passed,
and avoids contacting the external IP services when not listening.

Advertising non-listening nodes is just addr message spam.
It doesn't help the network, in fact it hurts the network,
and it also hurts user's privacy.

Advertising far out of sync nodes doesn't help the network—
they can't even forward (most) transactions and wastes nodes
outbound slots.

Change up/down increment in UI to 0.001 BTC (issue #760)

Dummy widget for preserving window state after restoring from system tray

Took some build dependincies off

*Fix Minimize to the tray instead of the taskbar

Restored changes

Avoid advertising the node's address when it is not listening or IsInitialBlockDownload().

This also avoids flushing setAddrKnown until 24 hours has passed,
and avoids contacting the external IP services when not listening.

Advertising non-listening nodes is just addr message spam.
It doesn't help the network, in fact it hurts the network,
and it also hurts user's privacy.

Advertising far out of sync nodes doesn't help the network—
they can't even forward (most) transactions and wastes nodes
outbound slots.

Change up/down increment in UI to 0.001 BTC (issue #760)

miniupnpc Porfile removed; new and improved macdeployqtplus

* My patch for miniupnpc has made it into the latest MacPorts release: https://trac.macports.org/ticket/31354
* Documentation has been changed appropriately
* New pure-Python macdeployqt; leverages all problems with the stock macdeployqt

Mac deploy tool: make dylibs writeable when copying into app bundle, so they can be stripped/nametool'ed

fetch translations from transifex

* fixes issue #742
* new translations: cs_CZ fa fi fr_FR hr pl ro_RO sv tr

Restructure credit transaction decomposition (solves issue #689)

When a transaction has multiple outputs that go to the wallet, list these
as multiple transactions in the UI. This is also applied to generated
(coinbase) transactions. Also makes the code shorter and easier
to understand.

Have bitcoind recommend a secure RPC password. Increase invalid password delay.

Help users avoid insecure configurations a bit by recommending a
secure RPC password and increasing the incorrect password delay.

This may open up a RPC DOS for users with exposed RPC ports and
short passwords. Since users shouldn't have exposed RPC ports OR
short passwords, the DOS risk is preferable to the compromise
risk.

Also logs the client IP address for incorrect attempts.

-bip16 option (default: 1) to support / not support BIP 16. And bumped default BIP16 switchover date from Feb 15 to Mar 1

Make transactions with extra data in their scriptSig's non-standard.

Unit tests for the GetArg() methods

New GetArg features: allow --, and booleans can be -foo or -nofoo

Look for flushwallet/listen/irc/dnsseed/upnp instead of noflushwallet/etc. And switch default for irc to 0.

Increase client version to 0.6

Update all copyrights to 2012

Update zlib and libpng (previous version had a security issue)

Split smaller dependencies out of gitian-win32 into deps-win32

Update doc/release-process.txt

Update readme-qt.rst for 0.6.0

- Remove features list (no longer makes sense)
- Document USE_QRCODE
a5a5d3c

@laanwj laanwj closed this Mar 24, 2012

@coblee coblee pushed a commit to litecoin-project/litecoin that referenced this issue Jul 17, 2012

@laanwj laanwj Restructure credit transaction decomposition (solves issue #689)
When a transaction has multiple outputs that go to the wallet, list these
as multiple transactions in the UI. This is also applied to generated
(coinbase) transactions. Also makes the code shorter and easier
to understand.
ac5f7dd

@jpentland jpentland pushed a commit to jpentland/bitcoin that referenced this issue Feb 7, 2016

@UdjinM6 @schinzelh UdjinM6 + schinzelh more dashpay.io->dash.org
Closes #689
826cba7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment