Prefer coins that have fewer ancestors, sanity check txn before ATMP

instagibbs commented at 4:09 am on December 2, 2016: member

This does a few extra possible runs of SelectCoinsMinConf, each with larger allowable mempool ancestor numbers, up to the acceptable mempool limit. As long as each input is not above this value, it passes. This means the sum of the history could actually be larger.

This is why I then catch the transaction at the end of CreateTransaction, and directly check. It simply fails, and does not retry. This logic is gated by a new command line argument -rejectlongwalletchains to regain previous behavior. We can most likely remove this check once we have better rebroadcasting systems.

fanquake added the label Wallet on Dec 2, 2016

fanquake added the label Mempool on Dec 2, 2016

sipa commented at 4:58 am on December 2, 2016: member

There should probably be a function in the place where mempool acceptance is decided (main.cpp now, validation.cpp after The Main Split) to do preliminary checking like thid.

gmaxwell commented at 2:52 pm on December 2, 2016: contributor

Probably not for now but when I read this code I wondered if we need an “available balance”– e.g. the sum of the coins that selectcoins would consider using… so that transaction failures due to this (and spendunconfirmed set off) are more explicable.

gmaxwell commented at 2:53 pm on December 2, 2016: contributor

FWIW, I think this should be backported.

instagibbs commented at 4:45 pm on December 2, 2016: member

While writing tests I found that this logic somehow is falling through on non-default ancestorcount limit. Debugging.

in src/wallet/wallet.cpp: in 98a1649bc3 outdated

1956@@ -1957,6 +1957,20 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const
1957             if (nDepth == 0 && !pcoin->InMempool())
1958                 continue;
1959 
1960+            LockPoints lp;
1961+            CTxMemPoolEntry entry(*pcoin, 0, 0, 0, 0, false, 0, false, 0, lp);

sdaftuar commented at 5:00 pm on December 2, 2016:

Since we’ve already checked that pcoin is in the mempool, we don’t need to do any of this – we can just use the cached values. (I think the only way the calculation you do below can fail is if the limits are violated during a reorg.)

instagibbs force-pushed on Dec 2, 2016

instagibbs renamed this:
~~Don't consider coins available if too many ancestors in mempool~~
Prefer coins that have fewer ancestors, sanity check txn before ATMP
on Dec 2, 2016

instagibbs force-pushed on Dec 2, 2016

instagibbs commented at 9:02 pm on December 2, 2016: member

Updated the pull request to new design discussed on IRC.

in src/wallet/wallet.h: in 0160c530ec outdated

399@@ -400,6 +400,7 @@ class CWalletTx : public CMerkleTx
400     bool IsEquivalentTo(const CWalletTx& tx) const;
401 
402     bool InMempool() const;
403+    uint64_t GetMempoolAncestorCount() const;

sdaftuar commented at 9:28 pm on December 2, 2016:

nit: add documentation to this function that the tx must be in the mempool in order to call?

in src/wallet/wallet.cpp: in 0160c530ec outdated

2047@@ -2042,6 +2048,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
2048         if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs))
2049             continue;
2050 
2051+        if (pcoin->InMempool() && pcoin->GetMempoolAncestorCount() > nMaxAncestors)

sdaftuar commented at 9:33 pm on December 2, 2016:

Perhaps worth a comment here that we’re assuming AvailableCoins() has already filtered out any unconfirmed and not-in-mempool coins?

instagibbs commented at 10:50 pm on December 2, 2016:

Inequality here is wrong, should be >= I believe. This will never fire as-is. I should be able to easily test this fix.

in src/wallet/wallet.cpp: in 82c4fe2eeb outdated

2557@@ -2558,6 +2558,21 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2558         }
2559     }
2560 
2561+    // Lastly, ensure it doesn't have too many ancestors

sdaftuar commented at 9:36 pm on December 2, 2016:

nit: perhaps this comment should be “Lastly, ensure this tx will pass the mempool’s chain limits”

in src/wallet/wallet.cpp: in 82c4fe2eeb outdated

2567+    size_t nLimitAncestorSize = GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000;
2568+    size_t nLimitDescendants = GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT);
2569+    size_t nLimitDescendantSize = GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000;
2570+    std::string errString;
2571+    if (!mempool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
2572+        strFailReason = _("Transaction has too long of a mempool chain");

sdaftuar commented at 9:38 pm on December 2, 2016:

The errString filled in by CMPA might be useful for debugging issues (eg a LogPrintf() so that someone debugging could inspect), though i see we don’t log any info about the transaction we tried to generate, so maybe not worth it…

sdaftuar commented at 9:44 pm on December 2, 2016: member

Code review ack, just a couple nits. Will test.

instagibbs force-pushed on Dec 2, 2016

instagibbs commented at 10:18 pm on December 2, 2016: member

I also think this is good for backport.

in src/wallet/wallet.cpp: in 9cdba4b999 outdated

2047@@ -2042,6 +2048,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
2048         if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs))
2049             continue;
2050 
2051+        if (pcoin->GetMempoolAncestorCount() > nMaxAncestors)

sdaftuar commented at 10:24 pm on December 2, 2016:

I think you need the pcoin->InMempool() check you had before, in case you’re looking at in-chain coins?

MarcoFalke added this to the milestone 0.13.2 on Dec 2, 2016

MarcoFalke added the label Needs backport on Dec 2, 2016

instagibbs force-pushed on Dec 2, 2016

gmaxwell commented at 7:11 pm on December 4, 2016: contributor

Looks good! – so, I think the additional test in “Don’t return success” should be controlled by an option, since the failure is only temporary (at least once we fix the rebroadcast bug) and many callers handle failure to create a transaction poorly. I might even go as far as to suggest that this option be set in 0.13.2 to continue to create the transaction because that would be less of an API change. Perhaps @sdaftuar or @laanwj has an opinion on that?

Other than that, utACK.

instagibbs force-pushed on Dec 4, 2016

instagibbs commented at 7:45 pm on December 4, 2016: member

Couple of fixes/changes:

if (pcoin->InMempool() && pcoin->GetMempoolAncestorCount() > nMaxAncestors)

is now

if (pcoin->InMempool() && pcoin->GetMempoolAncestorCount() >= nMaxAncestors)

so it actually filters correctly on the very last call of SelectCoinsMinConf.

I also revamped the tests to catch the various cases. @gmaxwell we’ll have to catch that logic in two places, SelectCoinsMinConf(at the last instance of it) and at the end of CreateTransaction. I suppose once rebroadcasting is fixed, we can set the default to “false” and let it take care of it. I’ll wait to see what others are thinking. I think current behavior is horrendous :)

gmaxwell commented at 2:42 am on December 5, 2016: contributor

Current behavior is horrendous indeed. Thank you for working on this.

sdaftuar commented at 7:59 pm on December 5, 2016: member

@gmaxwell No strong opinion on the issue of default behavior for 0.13.2, but given that you currently have to restart your node in order to rebroadcast, I’d lean towards making the default behavior be to enforce this new restriction, rather than create a tx that doesn’t make it to the mempool.

in src/wallet/wallet.cpp: in b077aa7d7a outdated

2047@@ -2042,6 +2048,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
2048         if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs))
2049             continue;
2050 
2051+        if (pcoin->InMempool() && pcoin->GetMempoolAncestorCount() >= nMaxAncestors)

sdaftuar commented at 8:35 pm on December 5, 2016:

@morcos and I were discussing, I think this would make more sense to limit based on both ancestor and descendant count, something like:

0if (pcoin->InMempool() && max(pcoin->GetMempoolAncestorCount(), pcoin->GetMempoolDescendantCount()) >= nMaxChain)
1    continue;

Otherwise, if you have an unspent output with 1 ancestor but 25 descendants, you (needlessly) could fail on one of the early calls.

in src/wallet/wallet.cpp: in b077aa7d7a outdated

2184-        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, vCoins, setCoinsRet, nValueRet));
2185+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 6, 0, vCoins, setCoinsRet, nValueRet) ||
2186+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, 0, vCoins, setCoinsRet, nValueRet) ||
2187+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nLimitAncestors/4, vCoins, setCoinsRet, nValueRet)) ||
2188+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nLimitAncestors/2, vCoins, setCoinsRet, nValueRet)) ||
2189+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nLimitAncestors, vCoins, setCoinsRet, nValueRet));

sdaftuar commented at 8:39 pm on December 5, 2016:

Instead of N/4, N/2, and N for the ancestor/chain limit passed in to SCMC, how about: 2, 4, N/2, N? (And set N = min(limitancestorcount, limitdescendantcount), assuming we go with ancestor and descendant limiting as I mentioned in another comment.) This way we first try utxo’s from transactions that have no ancestors or descendants, which should provide a strong preference for not creating long chains.

instagibbs force-pushed on Dec 5, 2016

instagibbs commented at 9:59 pm on December 5, 2016: member

@sdaftuar made sense to me, done

laanwj commented at 6:09 am on December 6, 2016: member

but given that you currently have to restart your node in order to rebroadcast

Why is that so? You can’t take the tx hex and send it through sendrawtransaction in this case?

in src/wallet/wallet.h: in f74f763023 outdated

687@@ -685,7 +688,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
688      * completion the coin set and corresponding actual target value is
689      * assembled
690      */
691-    bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, std::vector<COutput> vCoins, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const;
692+    bool SelectCoinsMinConf(const CAmount& nTargetValue, const int nConfMine, const int nConfTheirs, const uint64_t nMaxAncestors, std::vector<COutput> vCoins, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const;

sipa commented at 6:47 am on December 6, 2016:

Specifying const for a pass-by-value argument has no effect in the declaration. You can add const in the function definition if you want to protect yourself from accidentally modifying the variable even when the definition doesn’t have const.

in src/wallet/wallet.cpp: in f74f763023 outdated

2188-        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 6, vCoins, setCoinsRet, nValueRet) ||
2189-        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, vCoins, setCoinsRet, nValueRet) ||
2190-        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, vCoins, setCoinsRet, nValueRet));
2191+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 6, 0, vCoins, setCoinsRet, nValueRet) ||
2192+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, 0, vCoins, setCoinsRet, nValueRet) ||
2193+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, 2, vCoins, setCoinsRet, nValueRet)) ||

sipa commented at 6:47 am on December 6, 2016:

What if nLimitAncestors is less than 4?

gmaxwell commented at 6:58 am on December 6, 2016:

It calls Selectmincoins redundantly a bit… this doesn’t seem like a big deal to me.

in src/wallet/wallet.cpp: in 8f1ae0c931 outdated

2564@@ -2565,6 +2565,20 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2565         }
2566     }
2567 
2568+    // Lastly, ensure this tx will pass the mempool's chain limits

sipa commented at 6:49 am on December 6, 2016:

Can you abstract this fragment out, and turn it into a CTxMempool method?

instagibbs commented at 4:40 pm on December 6, 2016:

I don’t think is likely to save us many lines seeing as CMPA is called with these parameters exactly once elsewhere(and we’d still have to pass entry), and further this block may get removed if #9290 or something like it happens is merged.

gmaxwell commented at 6:57 am on December 6, 2016: contributor

but given that you currently have to restart your node in order to rebroadcast

How about we fix that? (#9290)

sdaftuar commented at 2:47 pm on December 6, 2016: member

@laanwj Regarding rebroadcast, I meant without doing something herculean – I think with current behavior you’d have to first find the txid that was committed but then not broadcast (since the RPC call will just return failure), and then rebroadcast it using something like sendrawtransaction. But anyway it should be moot after #9290, which I think makes sense.

sdaftuar commented at 3:55 pm on December 6, 2016: member

Moderately-tested ACK 2da773454c02990665ecd1b03f60a6ab09bdf22d

I tried to test this a couple different ways. One simple demonstration of this PR’s benefit was in a situation where you have 2 (confirmed) UTXO’s in your wallet, and you’re sending lots of small valued transactions to addresses outside your wallet. Before this PR, the test would consistently fail at the 26th spend (with the ensuing chaos of the rpc call failure, while the transaction was still committed). After this PR, I’m able to consistently create 50 transactions before failure, which is optimal.

in qa/rpc-tests/wallet.py: in 2da773454c outdated

399+                assert(False)
400+            except JSONRPCException as e:
401+                assert("Insufficient funds" in e.error['message'])
402+            self.nodes[0].lockunspent(True, [{"txid":txids[(i+1)%2], "vout":0}])
403+
404+        assert_equal(node0_balance, self.nodes[0].getbalance("*", 0))

morcos commented at 5:12 pm on December 6, 2016:

Overall I find this test a bit hard to follow. I wonder if you could make it a bit clearer.

It does a good job of testing that the send failures don’t leave funds unavailable (b/c the transaction isn’t committed), but I’m not sure that’s even what @gmaxwell wants, although I do think it is a good idea. But it doesn’t test the hopefully more optimal behavior of being less likely to create these long chains.

Also, please , please, please, please don’t use getbalance("*"), you can use getbalance() + getunconfirmedbalance() if you need the total including mempool transactions…

instagibbs commented at 6:00 pm on December 6, 2016:

Particularly I was interested in checking it errors out when, and how. So I build two long chains, start checking that sending will fail due to final checks when combining the chains, not “lack of coins”, then once it gets long enough, both coins are already too long to include.

This test might be too obscure/specific, but it helped convince me that the code is doing what I wanted it to do. I should probably add a simple test like @sdaftuar ’s where it shows it preferentially choosing shorter chained outputs first.

please don’t use getbalance("*")

Leftover, sorry. Will fix.

morcos approved

morcos commented at 5:46 pm on December 6, 2016: member

code review ACK modulo nits about the test

instagibbs force-pushed on Dec 6, 2016

instagibbs commented at 6:09 pm on December 6, 2016: member

Added init flag -rejectlongwalletchains which is by default True. When set to false only the preferential SelectCoinsMinConf behavior remains, with the last iteration accepting any length.

in src/wallet/wallet.cpp: in 6ba348b694 outdated

2190-        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, vCoins, setCoinsRet, nValueRet) ||
2191-        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, vCoins, setCoinsRet, nValueRet));
2192+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 6, 0, vCoins, setCoinsRet, nValueRet) ||
2193+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, 0, vCoins, setCoinsRet, nValueRet) ||
2194+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, 2, vCoins, setCoinsRet, nValueRet)) ||
2195+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, 4, vCoins, setCoinsRet, nValueRet)) ||

gmaxwell commented at 6:26 pm on December 6, 2016:

So now when nLimitAncestors is set to 3 it will violate it here?

instagibbs commented at 6:29 pm on December 6, 2016:

Perhaps just pick larger denominators? n/10, n/5, n/2, n(or inf).

sdaftuar commented at 6:49 pm on December 6, 2016:

@gmaxwell We’re already not guaranteed to find an optimal spend, this is just best-efforts anyway, so in my opinion it’s not worth trying to optimize further for users using unusual values of these policy limits, which we already discourage changes to by hiding away in help-debug.

At any rate I think having explicit small values here is overall a win, in case someone were using non-default arguments for nLimitAncestors – this ensures that the coin selection is always first going to try coins that have no ancestors/descendants.

gmaxwell commented at 7:33 pm on December 6, 2016:

0, (N+1)/4, (N+1)/2, N (or inf) gives all unique results for everything except 1 and 2… so the user would waste some computation for 1,2.

gmaxwell commented at 9:16 pm on December 6, 2016:

Restating my suggestion: It’s better to have more small number attempts because the large attempts will intermix chains and trash your short chains. So min(2,limit/4), min(4, limit/3), limit/2, limit would be a better pick… for example

morcos commented at 9:19 pm on December 6, 2016:

ok sounds good to me

instagibbs commented at 9:38 pm on December 6, 2016:

done

instagibbs force-pushed on Dec 6, 2016

gmaxwell commented at 3:03 am on December 7, 2016: contributor

This is the prettiest blue bike-shed ever. ACK.

sdaftuar commented at 9:02 pm on December 8, 2016: member

Note that if someone uses -rejectlongwalletchains=0, then in my simple use-case above (2 confirmed UTXOs that you’re repeatedly making small spends from) this current code will unnecessarily produce a too-long-chain, because we don’t try calling SCMC with nlimitancestors.

After #9290 and #9302 I think this won’t be a big deal, so given that we’re defaulting this to true I don’t think it matters much, and this shed has been repainted so many times we’re probably running out of paint. re-ACK cb6a6be.

in src/wallet/wallet.cpp: in cb6a6bec0a outdated

1748+
1749+uint64_t CWalletTx::GetMempoolDescendantCount() const
1750+{
1751+    LOCK(mempool.cs);
1752+    return mempool.mapTx.find(GetHash())->GetCountWithDescendants();
1753+}

luke-jr commented at 0:48 am on December 9, 2016:

These seem like they would be more appropriate on CTxMemPool?

sipa commented at 1:15 am on December 13, 2016:

Agree with @luke-jr, but I think we need something higher-level anyway (see further).

in src/wallet/wallet.cpp: in cb6a6bec0a outdated

2053@@ -2042,6 +2054,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
2054         if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs))
2055             continue;
2056 
2057+        if (pcoin->InMempool() && std::max(pcoin->GetMempoolAncestorCount(), pcoin->GetMempoolDescendantCount()) >= nMaxAncestors)

luke-jr commented at 2:20 am on December 9, 2016:

Why would we compare the descendant count to the max ancestors? Shouldn’t it be compared to max descendants?

instagibbs commented at 12:42 pm on December 9, 2016:

misnomer, nMaxAncestors is the minimum of the two parameters. Will fix.

sipa commented at 1:13 am on December 13, 2016:

Given that we don’t hold mempool.cs here, the transaction could be removed from the mempool in between the pcoin->InMempool() and pcoin->GetMempoolAncestorCount() calls, causing a segfault in the latter. Please abstract this whole conditional out into a method on CTxMempool, which takes the limit value and the txid, and returns whether the transaction is in the pool and satisfies the limits.

in src/wallet/wallet.cpp: in cb6a6bec0a outdated

2181@@ -2167,10 +2182,16 @@ bool CWallet::SelectCoins(const vector<COutput>& vAvailableCoins, const CAmount&
2182             ++it;
2183     }
2184 
2185+    size_t nLimitAncestors = std::min(GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT), GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT));
2186+    bool frejectLongChains = GetBoolArg("-rejectlongwalletchains", DEFAULT_REJECT_TOOLONG_CHAINS);

luke-jr commented at 2:22 am on December 9, 2016:

Probably should capitalise the ‘R’ in the var name.

luke-jr commented at 2:26 am on December 9, 2016:

Might be best to have the default const, variable, and argument be more closely matched.

Suggest DEFAULT_WALLET_REJECT_LONG_CHAINS, fRejectLongChains, and -walletrejectlongchains

in src/wallet/wallet.cpp: in cb6a6bec0a outdated

2192+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 6, 0, vCoins, setCoinsRet, nValueRet) ||
2193+        SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, 0, vCoins, setCoinsRet, nValueRet) ||
2194+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, std::min((size_t)2, nLimitAncestors/4), vCoins, setCoinsRet, nValueRet)) ||
2195+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, std::min((size_t)4, nLimitAncestors/3), vCoins, setCoinsRet, nValueRet)) ||
2196+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nLimitAncestors/2, vCoins, setCoinsRet, nValueRet)) ||
2197+        (bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, frejectLongChains ? nLimitAncestors : std::numeric_limits<uint64_t>::max(), vCoins, setCoinsRet, nValueRet));

luke-jr commented at 2:23 am on December 9, 2016:

Shouldn’t nLimitAncestors here be (nLimitAncestors - 1), because otherwise the new transaction would be 1 beyond the limit?

luke-jr commented at 2:23 am on December 9, 2016:

Even if frejectLongChains is set, shouldn’t we prefer coins <nLimitAncestors when possible?

instagibbs commented at 12:58 pm on December 9, 2016:

the count counts the coin itself in the chain length, so I think it’s right as-is. >= is the check to reject.

in src/wallet/wallet.cpp: in cb6a6bec0a outdated

2572+        CTxMemPoolEntry entry(txNew, 0, 0, 0, 0, false, 0, false, 0, lp);
2573+        CTxMemPool::setEntries setAncestors;
2574+        size_t nLimitAncestors = GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT);
2575+        size_t nLimitAncestorSize = GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000;
2576+        size_t nLimitDescendants = GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT);
2577+        size_t nLimitDescendantSize = GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000;

luke-jr commented at 2:29 am on December 9, 2016:

Might make sense to make a bool CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, std::string &errString, bool fSearchForParents = true) const; for this common code.

OTOH, we should probably split the policy logic out of ATMP, so this refactoring may not make sense long-term.

instagibbs force-pushed on Dec 9, 2016

instagibbs commented at 1:43 pm on December 9, 2016: member

Anticipating other changes to wallet #9302 https://github.com/bitcoin/bitcoin/pull/9290 , I changed the default for rejecting wallet transactions to false, and took the opportunity to put on a couple more layers of paint with renaming suggestions by @luke-jr , made a much simpler test that just checks for wallet coin preferential treatment, always have the wallet attempt an at-limit coin selection and then followup with an anything goes attempt if arguments allow.

gmaxwell commented at 9:23 pm on December 10, 2016: contributor

@instagibbs Test fails– otherwise looking fine to me.

instagibbs force-pushed on Dec 10, 2016

instagibbs commented at 9:43 pm on December 10, 2016: member

forgot to rename the param in description, fixed

in src/wallet/wallet.cpp: in 5d7edcdbfb outdated

3383@@ -3347,6 +3384,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3384                                                             CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE)));
3385     strUsage += HelpMessageOpt("-paytxfee=<amt>", strprintf(_("Fee (in %s/kB) to add to transactions you send (default: %s)"),
3386                                                             CURRENCY_UNIT, FormatMoney(payTxFee.GetFeePerK())));
3387+    strUsage += HelpMessageOpt("-walletrejectlongchains", strprintf(_("Wallet will not create transactions that violate mempool chain limits (default: %u"), DEFAULT_WALLET_REJECT_LONG_CHAINS));

MarcoFalke commented at 2:25 pm on December 11, 2016:

I don’t think it makes sense to expose such a detail to every user. (Note that all the -limit<whatever> options are only displayed to advanced users.)

MarcoFalke commented at 2:30 pm on December 11, 2016:

In light of search for a general solution to the problem, you might want to consider introducing an option that makes the wallet ignore whatever mempool policies are in place right now.

When this option is enabled, we get the current behavior, i.e. transactions might not be accepted to the mempool and might not be relayed. However, when the option is disabled and the mempool policy is considered during transaction creation, every transaction that is created also is accepted to the mempool. If no such transaction can be created, the call fails.

instagibbs commented at 5:50 pm on December 11, 2016:

I believe the consensus is to have all temporary failures return a txid, and rebroadcast the transaction when it can, entering it into the mempool.

re:detailing to every user, fair enough. I’ve moved it under showDebug, which I believe is what you meant.

MarcoFalke commented at 2:30 pm on December 11, 2016: member

Concept ACK

instagibbs force-pushed on Dec 11, 2016

morcos commented at 5:29 pm on December 12, 2016: member

Tested ACK 63c17b8

However I still don’t really love the test. For instance the test doesn’t work if you haven’t set the ancestor limit lower than the default. It’s in general tricky to test behavior which depends on the vagaries of coin selection, but one simple improvement to this test would be to send the coins to a different nodes address so then we only have the change to consider for subsequent spends and not the outputs themselves. This would make it so you could reliably send (number of original outputs * chain limits) transactions as long as the change is large compared to the value you’re sending.

However it would be nice if we could stop rebasing this PR and get it merged, so if you do want to change the test, please don’t modify the other two commits…

gmaxwell commented at 1:01 am on December 13, 2016: contributor

ACK.

in src/wallet/wallet.cpp: in 63c17b8216 outdated

2566@@ -2545,6 +2567,21 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2567         }
2568     }
2569 
2570+    if (GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
2571+        // Lastly, ensure this tx will pass the mempool's chain limits

sipa commented at 1:20 am on December 13, 2016:

I don’t care that it’s called only once - this just doesn’t seem like code that belongs in the wallet (which shouldn’t need to know about the mempool’s policy limits as much as possible). Having it abstracted probably makes it usable for other purposes too.

Also, #9290 does not mean we can’t do belt and suspender checks to avoid producing long chains. Not adding such a transaction to the wallet (and mempool) is still superior to just adding it to the wallet and relying on retransmit to get it to go through at a later point in time.

TheBlueMatt commented at 9:46 pm on December 19, 2016:

Agreed, though in the interest of backporting and getting 0.13.2 out cleaing up the interface here probably isnt the priority. Lets do it in another pr.

in src/wallet/wallet.h: in 63c17b8216 outdated

52@@ -53,6 +53,8 @@ static const CAmount MIN_CHANGE = CENT;
53 static const bool DEFAULT_SPEND_ZEROCONF_CHANGE = true;
54 //! Default for -sendfreetransactions
55 static const bool DEFAULT_SEND_FREE_TRANSACTIONS = false;
56+//! Default for -walletrejectlongchains
57+static const bool DEFAULT_WALLET_REJECT_LONG_CHAINS = false;

sipa commented at 1:21 am on December 13, 2016:

Why false?

MarcoFalke commented at 1:21 pm on December 14, 2016:

It is the least change in behavior, so makes sense in case this is still considered for backport.

instagibbs force-pushed on Dec 13, 2016

instagibbs commented at 2:28 pm on December 13, 2016: member

@sipa Replaced SCMC mempool check with more generalized check in CTxMempool.

re: default I disagree, I think that the default behavior here makes more sense unless the user has a way of probing for something like GetBalanceWithinChainLimits().

SelectCoinsMinConf: Prefer coins with fewer ancestors 0b2294a980

CreateTransaction: Don't return success with too-many-ancestor txn 5882c099d9

Test for fix of txn chaining in wallet af9bedbff6

in src/txmempool.h: in 383c0854d5 outdated

604@@ -605,6 +605,9 @@ class CTxMemPool
605     /** Expire all transaction (and their dependencies) in the mempool older than time. Return the number of removed transactions. */
606     int Expire(int64_t time);
607 
608+    /** Returns false if the transaction is in the mempool and not within the chain limit specified. */
609+    bool CoinWithinLimit(const uint256& txid, size_t chainLimit) const;

morcos commented at 2:33 pm on December 13, 2016:

Should we call this something other than CoinWithinLimit? Seems like coins are not something that the mempool is aware of.

instagibbs commented at 2:36 pm on December 13, 2016:

TransactionWithinChainLimit?

instagibbs force-pushed on Dec 13, 2016

in src/txmempool.cpp: in 0b2294a980 outdated

1141@@ -1142,3 +1142,10 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<uint256>* pvNoSpendsRe
1142     if (maxFeeRateRemoved > CFeeRate(0))
1143         LogPrint("mempool", "Removed %u txn, rolling minimum fee bumped to %s\n", nTxnRemoved, maxFeeRateRemoved.ToString());
1144 }
1145+
1146+bool CTxMemPool::TransactionWithinChainLimit(const uint256& txid, size_t chainLimit) const {
1147+    LOCK(cs);
1148+    if (exists(txid) && std::max(mapTx.find(txid)->GetCountWithAncestors(), mapTx.find(txid)->GetCountWithDescendants()) >= chainLimit)

sipa commented at 7:56 am on December 19, 2016:

You’re doing 3 map lookups here. What about

0auto it = mapTx.find(txid);
1return it != mapTx.end() && it->GetCountWithAncestors() < chainLimit &&
2    it->GetCountWithDescendants() < chainLimit;

sipa commented at 8:02 am on December 19, 2016: member

utACK 5882c099d9af6e8566d0bf46fb1da424a4373bf8, with one nit.

instagibbs commented at 2:51 pm on December 19, 2016: member

fixed @sipa nit

laanwj commented at 3:47 pm on December 19, 2016: member

Travis doesn’t agree with your change, it seems.

instagibbs force-pushed on Dec 19, 2016

instagibbs commented at 3:55 pm on December 19, 2016: member

@laanwj yes the logic made no sense, it will reject anything not in mempool rather than anything in mempool with too long a chain. Fixed.

in src/wallet/wallet.h: in ff79329ccf outdated

686@@ -685,7 +687,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
687      * completion the coin set and corresponding actual target value is
688      * assembled
689      */
690-    bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, std::vector<COutput> vCoins, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const;
691+    bool SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int nConfTheirs, uint64_t nMaxAncestors, std::vector<COutput> vCoins, std::set<std::pair<const CWalletTx*,unsigned int> >& setCoinsRet, CAmount& nValueRet) const;

TheBlueMatt commented at 9:28 pm on December 19, 2016:

nit: you forgot to const-ify the first two ints which you did in the .cpp

in src/wallet/wallet.cpp: in ff79329ccf outdated

2554@@ -2545,6 +2555,21 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2555         }
2556     }
2557 
2558+    if (GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
2559+        // Lastly, ensure this tx will pass the mempool's chain limits
2560+        LockPoints lp;
2561+        CTxMemPoolEntry entry(txNew, 0, 0, 0, 0, false, 0, false, 0, lp);

TheBlueMatt commented at 10:07 pm on December 19, 2016:

Note that by not filling in the sigOpsCost here we dont have that information for the virtual transaction size, so you could still run over the limit here. I don’t think its a big deal, but we should fix when we clean things up in 0.14 as suggested two lines up.

TheBlueMatt approved

TheBlueMatt commented at 10:10 pm on December 19, 2016: member

utACK ff79329ccfe3ee4387b9de72d82431c7b8132272 some comments but none worth delaying 0.13.2 for.

reduce number of lookups in TransactionWithinChainLimit cee16123f5

in src/txmempool.cpp: in ff79329ccf outdated

1144@@ -1145,7 +1145,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<uint256>* pvNoSpendsRe
1145 
1146 bool CTxMemPool::TransactionWithinChainLimit(const uint256& txid, size_t chainLimit) const {
1147     LOCK(cs);
1148-    if (exists(txid) && std::max(mapTx.find(txid)->GetCountWithAncestors(), mapTx.find(txid)->GetCountWithDescendants()) >= chainLimit)
1149-        return false;
1150-    return true;
1151+    auto it = mapTx.find(txid);
1152+    return it == mapTx.end() || (it->GetCountWithAncestors() < chainLimit &&
1153+    it->GetCountWithDescendants() < chainLimit);

gmaxwell commented at 1:42 am on December 20, 2016:

This really should be indented.

instagibbs force-pushed on Dec 20, 2016

btcdrak approved

laanwj merged this on Dec 20, 2016

laanwj closed this on Dec 20, 2016

laanwj referenced this in commit 5a70572049 on Dec 20, 2016

laanwj removed the label Needs backport on Dec 20, 2016

laanwj commented at 12:41 pm on December 20, 2016: member

Backported in #9382, removing label

codablock referenced this in commit dc1df137e3 on Jan 18, 2018

andvgal referenced this in commit 7706fd6127 on Jan 6, 2019

CryptoCentric referenced this in commit b584542189 on Feb 26, 2019

MarcoFalke locked this on Sep 8, 2021

Prefer coins that have fewer ancestors, sanity check txn before ATMP #9262