Add RPC call abandontransaction #7312

Merged
merged 4 commits into from Jan 13, 2016

Conversation

Projects
None yet
8 participants
@morcos
Member

morcos commented Jan 7, 2016

Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined. abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction. All dependent transactions in the wallet will also be marked as abandoned.

@laanwj also for 0.12

This is the basic functionality.
There are more things to add though. I have an RPC test in progress, but if anyone else wants to work on the remaining items, please do:

  • Return abandoned status in listtransactions
  • Return abandoned status in GUI
  • Fix any issues with how abandoned txs should sort
  • Add a way to abandon transactions from GUI

I built this off of #7306 to make sure the tests would work correctly.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 7, 2016

Member

This is way too late for 0.12...

Member

luke-jr commented Jan 7, 2016

This is way too late for 0.12...

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 7, 2016

Member

@luke-jr I don't think we can release 0.12 without this. With mempool eviction combined with the new behaviour that unconfirmed txs still tie up the coins they spend even if the txs are not in the mempool, users may end up with permanently unspendable coins frequently.

We discussed making this change in conjunction with the other changes back in November, but it appears to have slipped through the cracks.

Member

morcos commented Jan 7, 2016

@luke-jr I don't think we can release 0.12 without this. With mempool eviction combined with the new behaviour that unconfirmed txs still tie up the coins they spend even if the txs are not in the mempool, users may end up with permanently unspendable coins frequently.

We discussed making this change in conjunction with the other changes back in November, but it appears to have slipped through the cracks.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 7, 2016

Member

This situation isn't really any worse than the status quo from older versions...

Member

luke-jr commented Jan 7, 2016

This situation isn't really any worse than the status quo from older versions...

@laanwj laanwj added the Wallet label Jan 7, 2016

@jonasschnelli

View changes

src/wallet/wallet.cpp
- if (mit != mapWallet.end() && mit->second.GetDepthInMainChain() >= 0)
- return true; // Spent
+ if (mit != mapWallet.end()) {
+ if (mit->second.GetDepthInMainChain() > 0 || (mit->second.GetDepthInMainChain() == 0 && !mit->second.isAbandoned()))

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 7, 2016

Member

CMerkleTx::GetDepthInMainChain() is slightly expansive (and IsSpent() gets called a lot). Would caching mit->second.GetDepthInMainChain() make sense? Or do we expect the compiler does it internally (I don't think so)?

@jonasschnelli

jonasschnelli Jan 7, 2016

Member

CMerkleTx::GetDepthInMainChain() is slightly expansive (and IsSpent() gets called a lot). Would caching mit->second.GetDepthInMainChain() make sense? Or do we expect the compiler does it internally (I don't think so)?

This comment has been minimized.

@morcos

morcos Jan 7, 2016

Member

sure can fix

@morcos

morcos Jan 7, 2016

Member

sure can fix

@jonasschnelli

View changes

src/wallet/wallet.cpp
@@ -48,6 +48,8 @@ bool fSendFreeTransactions = DEFAULT_SEND_FREE_TRANSACTIONS;
*/
CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE);
+const uint256 CMerkleTx::abandonHash(uint256S("0000000000000000000000000000000000000000000000000000000000000001"));

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 7, 2016

Member

nit: I probably would do it also that way,... but, maybe a cleaner way would be to use CWalletTx::mapValue (a flexible kv store) which also would not require a db-scheme update.

@jonasschnelli

jonasschnelli Jan 7, 2016

Member

nit: I probably would do it also that way,... but, maybe a cleaner way would be to use CWalletTx::mapValue (a flexible kv store) which also would not require a db-scheme update.

This comment has been minimized.

@morcos

morcos Jan 7, 2016

Member

I debated doing that and maybe it would have been better, but I liked the way updating the hashBlock automatically made the transaction no longer abandoned with this approach.

@morcos

morcos Jan 7, 2016

Member

I debated doing that and maybe it would have been better, but I liked the way updating the hashBlock automatically made the transaction no longer abandoned with this approach.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 8, 2016

Member

I debated doing that and maybe it would have been better, but I liked the way updating the hashBlock automatically made the transaction no longer abandoned with this approach.

fair enough.

@jonasschnelli

jonasschnelli Jan 8, 2016

Member

I debated doing that and maybe it would have been better, but I liked the way updating the hashBlock automatically made the transaction no longer abandoned with this approach.

fair enough.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 7, 2016

Member

Concept ACK.
I agree it late for 0.12,... but I don't take a position on this (if it's saver to go with or without a such feature).

Would be nice to see some tests (especially when we tag this for 0.12).

Member

jonasschnelli commented Jan 7, 2016

Concept ACK.
I agree it late for 0.12,... but I don't take a position on this (if it's saver to go with or without a such feature).

Would be nice to see some tests (especially when we tag this for 0.12).

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 7, 2016

Member

Addressed @jonasschnelli's comment (thanks for quick review!) and reordered commits for simpler reading.

Member

morcos commented Jan 7, 2016

Addressed @jonasschnelli's comment (thanks for quick review!) and reordered commits for simpler reading.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 8, 2016

Member

oops, i didn't mean to push that RPC test yet, it was still in progress, but might as well leave it there for now...

Member

morcos commented Jan 8, 2016

oops, i didn't mean to push that RPC test yet, it was still in progress, but might as well leave it there for now...

@kanzure

View changes

src/wallet/rpcwallet.cpp
+ throw runtime_error(
+ "abandontransaction \"txid\"\n"
+ "\nMark in-wallet transaction <txid> as abandoned\n"
+ "This will mark this tx and all its in-wallet descendants as abandoned which will allow\n"

This comment has been minimized.

@kanzure

kanzure Jan 8, 2016

Contributor

consistency nit: all the other fHelp messages use "transaction" and "transactions" instead of "tx" and "txs".

@kanzure

kanzure Jan 8, 2016

Contributor

consistency nit: all the other fHelp messages use "transaction" and "transactions" instead of "tx" and "txs".

@kanzure

This comment has been minimized.

Show comment
Hide comment
@kanzure

kanzure Jan 8, 2016

Contributor

I think a name like "abandonlocaltransaction" or "abandonunconfirmedtransaction" could help to minimize ambiguity. But I would consider this a very low priority suggestion, because this RPC call is only available when the wallet is enabled.

Contributor

kanzure commented Jan 8, 2016

I think a name like "abandonlocaltransaction" or "abandonunconfirmedtransaction" could help to minimize ambiguity. But I would consider this a very low priority suggestion, because this RPC call is only available when the wallet is enabled.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 8, 2016

Member

abandonunconfirmedtransaction

How would you type that without any typos? I'd rather make the rpc short ("abandontx") and the documentation/GUI verbose.

Member

MarcoFalke commented Jan 8, 2016

abandonunconfirmedtransaction

How would you type that without any typos? I'd rather make the rpc short ("abandontx") and the documentation/GUI verbose.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 8, 2016

Member

Concept ACK 6d74a63154cca756d698d6022c694b21e7f43ac5

Member

MarcoFalke commented Jan 8, 2016

Concept ACK 6d74a63154cca756d698d6022c694b21e7f43ac5

@laanwj laanwj added this to the 0.12.0 milestone Jan 8, 2016

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 8, 2016

Member

Thanks for the review.
Addressed nits in RPC help and finalized the RPC test

Member

morcos commented Jan 8, 2016

Thanks for the review.
Addressed nits in RPC help and finalized the RPC test

@MarcoFalke

View changes

qa/pull-tester/rpc-tests.py
@@ -105,6 +105,7 @@
'prioritise_transaction.py',
'invalidblockrequest.py',
'invalidtxrequest.py',
+ 'abandonconflicts.py',

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 8, 2016

Member

abandonconflicts.py: No such file or directory

@MarcoFalke

MarcoFalke Jan 8, 2016

Member

abandonconflicts.py: No such file or directory

@MarcoFalke

View changes

qa/rpc-tests/abandonconflict.py
+
+ def setup_network(self):
+ self.nodes = []
+ self.nodes.append(start_node(0, self.options.tmpdir, ["-debug","-logtimemicros","-minrelaytxfee=0.00001"], binary="bitcoind"))

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 8, 2016

Member

Nit: Is the "-debug","-logtimemicros" part required?

Also the binary="bitcoind" part?

@MarcoFalke

MarcoFalke Jan 8, 2016

Member

Nit: Is the "-debug","-logtimemicros" part required?

Also the binary="bitcoind" part?

This comment has been minimized.

@sdaftuar

sdaftuar Jan 8, 2016

Member

I think we should drop the binary="bitcoind" part -- by default start_node can pick up the binary from the BITCOIND environment variable which is preferred when set.

@sdaftuar

sdaftuar Jan 8, 2016

Member

I think we should drop the binary="bitcoind" part -- by default start_node can pick up the binary from the BITCOIND environment variable which is preferred when set.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 8, 2016

Member

Reviewed-tests ACK 21702ff

@morcos Please fix the travis issue (file not found)

Member

MarcoFalke commented Jan 8, 2016

Reviewed-tests ACK 21702ff

@morcos Please fix the travis issue (file not found)

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 8, 2016

Member

oops sorry.
fixed typo and removed bitcoind argument from rpc test.

Member

morcos commented Jan 8, 2016

oops sorry.
fixed typo and removed bitcoind argument from rpc test.

+ balance = newbalance
+
+ # Send child tx again so its unabandoned
+ self.nodes[0].sendrawtransaction(signed2["hex"])

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 8, 2016

Member

Shouldn't resendwallettransactions do the same? I get the empty array on resendwallettransactions.

@MarcoFalke

MarcoFalke Jan 8, 2016

Member

Shouldn't resendwallettransactions do the same? I get the empty array on resendwallettransactions.

This comment has been minimized.

@morcos

morcos Jan 13, 2016

Member

I thought it made sense that abandoned transactions would not be automatically resent.
That way when you restart a bitcoind with transactions that you've tried to abandon, it wouldn't automatically try to send them all again

@morcos

morcos Jan 13, 2016

Member

I thought it made sense that abandoned transactions would not be automatically resent.
That way when you restart a bitcoind with transactions that you've tried to abandon, it wouldn't automatically try to send them all again

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 8, 2016

Member

QT-tested ACK d716866

screenshot from 2016-01-08 20-41-15
Nit: Qt balance != getwalletinfo

Member

MarcoFalke commented Jan 8, 2016

QT-tested ACK d716866

screenshot from 2016-01-08 20-41-15
Nit: Qt balance != getwalletinfo

+ wtx.nIndex = -1;
+ wtx.setAbandoned();
+ wtx.MarkDirty();
+ wtx.WriteToDisk(&walletdb);

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 11, 2016

Member

I think here we also need a NotifyTransactionChanged(this, wtx.GetHash(), CT_UPDATED); Otherwise the signal listeners (UI) can't recalculate the balance.

@jonasschnelli

jonasschnelli Jan 11, 2016

Member

I think here we also need a NotifyTransactionChanged(this, wtx.GetHash(), CT_UPDATED); Otherwise the signal listeners (UI) can't recalculate the balance.

+ BOOST_FOREACH(const CTxIn& txin, wtx.vin)
+ {
+ if (mapWallet.count(txin.prevout.hash))
+ mapWallet[txin.prevout.hash].MarkDirty();

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 11, 2016

Member

Probably also here:
NotifyTransactionChanged(this, txin.prevout.hash, CT_UPDATED);

@jonasschnelli

jonasschnelli Jan 11, 2016

Member

Probably also here:
NotifyTransactionChanged(this, txin.prevout.hash, CT_UPDATED);

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 11, 2016

Member

Tested ACK (also ack on a 0.12 bp, because it's a wallet only change)
Nits:

  • missing update signal (see above)
  • missing flag in listtransactions.
Member

jonasschnelli commented Jan 11, 2016

Tested ACK (also ack on a 0.12 bp, because it's a wallet only change)
Nits:

  • missing update signal (see above)
  • missing flag in listtransactions.
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 11, 2016

Member

Addressed the nits and a cosmetic change for GUI in: morcos#6

Member

jonasschnelli commented Jan 11, 2016

Addressed the nits and a cosmetic change for GUI in: morcos#6

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 11, 2016

Member

@jonasschnelli , thanks! I added your notify commit. I left the other changes for a separate pull.

Member

morcos commented Jan 11, 2016

@jonasschnelli , thanks! I added your notify commit. I left the other changes for a separate pull.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 11, 2016

Member

Ok updated after speaking to @jonasschnelli
I think this is good now

Member

morcos commented Jan 11, 2016

Ok updated after speaking to @jonasschnelli
I think this is good now

@PRabahy

This comment has been minimized.

Show comment
Hide comment
Contributor

PRabahy commented Jan 12, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 12, 2016

Member

@PRabahy sure, but I don't think this is part of this pull.

Member

MarcoFalke commented Jan 12, 2016

@PRabahy sure, but I don't think this is part of this pull.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 12, 2016

Collaborator

nit: couldn't this just be todo.erase(todo.begin()) to save the map lookup?

Collaborator

sdaftuar commented on src/wallet/wallet.cpp in 9e69717 Jan 12, 2016

nit: couldn't this just be todo.erase(todo.begin()) to save the map lookup?

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 13, 2016

I guess that makes sense. You don't want to hold on to any iterators here for longer as you're updating the data structure while pulling from it (the erase invalidates the iterator to the first element), but re-using the same iterator to remove the first element makes sense.

I guess that makes sense. You don't want to hold on to any iterators here for longer as you're updating the data structure while pulling from it (the erase invalidates the iterator to the first element), but re-using the same iterator to remove the first element makes sense.

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 13, 2016

Collaborator

Yeah, but this probably doesn't really matter though for this data structure, which won't have too much in it I think. Don't think this should hold up anything, can cleanup later.

Collaborator

sdaftuar replied Jan 13, 2016

Yeah, but this probably doesn't really matter though for this data structure, which won't have too much in it I think. Don't think this should hold up anything, can cleanup later.

morcos and others added some commits Jan 7, 2016

Add new rpc call: abandontransaction
Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined.  abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction.  All dependent transactions in the wallet will also be marked as abandoned.
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 13, 2016

Member

Addressed @laanwj's concern about clearly identifying constant

Member

morcos commented Jan 13, 2016

Addressed @laanwj's concern about clearly identifying constant

@laanwj laanwj merged commit d11fc16 into bitcoin:master Jan 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 13, 2016

Merge pull request #7312
d11fc16 [Wallet] Call notification signal when a transaction is abandoned (Jonas Schnelli)
df0e222 Add RPC test for abandoned and conflicted transactions. (Alex Morcos)
01e06d1 Add new rpc call: abandontransaction (Alex Morcos)
9e69717 Make wallet descendant searching more efficient (Alex Morcos)

laanwj added a commit that referenced this pull request Jan 13, 2016

Add RPC call abandontransaction
- Make wallet descendant searching more efficient
- Add new rpc call: abandontransaction

Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined.  abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction.  All dependent transactions in the wallet will also be marked as abandoned.

- Add RPC test for abandoned and conflicted transactions.
- [Wallet] Call notification signal when a transaction is abandoned

Github-Pull: #7312
Rebased-From: 9e69717 01e06d1 df0e222 d11fc16

morcos added a commit to morcos/bitcoin that referenced this pull request Jan 13, 2016

Add RPC call abandontransaction
- Make wallet descendant searching more efficient
- Add new rpc call: abandontransaction

Unconfirmed transactions that are not in your mempool either due to eviction or other means may be unlikely to be mined.  abandontransaction gives the wallet a way to no longer consider as spent the coins that are inputs to such a transaction.  All dependent transactions in the wallet will also be marked as abandoned.

- Add RPC test for abandoned and conflicted transactions.
- [Wallet] Call notification signal when a transaction is abandoned

Github-Pull: #7312
Rebased-From: 9e69717 01e06d1 df0e222 d11fc16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment