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

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

Merged
merged 4 commits into from Dec 20, 2016

Conversation

@instagibbs
Copy link
Member

commented Dec 2, 2016

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.

@sipa

This comment has been minimized.

Copy link
Member

commented Dec 2, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Dec 2, 2016

FWIW, I think this should be backported.

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2016

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

src/wallet/wallet.cpp Outdated
@@ -1957,6 +1957,20 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed, const
if (nDepth == 0 && !pcoin->InMempool())
continue;

LockPoints lp;
CTxMemPoolEntry entry(*pcoin, 0, 0, 0, 0, false, 0, false, 0, lp);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 2, 2016

Member

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 instagibbs force-pushed the instagibbs:toolong branch Dec 2, 2016

@instagibbs instagibbs changed the title Don't consider coins available if too many ancestors in mempool Prefer coins that have fewer ancestors, sanity check txn before ATMP Dec 2, 2016

@instagibbs instagibbs force-pushed the instagibbs:toolong branch Dec 2, 2016

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2016

Updated the pull request to new design discussed on IRC.

@sdaftuar
Copy link
Member

left a comment

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

src/wallet/wallet.h Outdated
@@ -400,6 +400,7 @@ class CWalletTx : public CMerkleTx
bool IsEquivalentTo(const CWalletTx& tx) const;

bool InMempool() const;
uint64_t GetMempoolAncestorCount() const;

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 2, 2016

Member

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

src/wallet/wallet.cpp Outdated
@@ -2042,6 +2048,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs))
continue;

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 2, 2016

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Dec 2, 2016

Author Member

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

src/wallet/wallet.cpp Outdated
@@ -2558,6 +2558,21 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
}
}

// Lastly, ensure it doesn't have too many ancestors

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 2, 2016

Member

nit: perhaps this comment should be "Lastly, ensure this tx will pass the mempool's chain limits"

src/wallet/wallet.cpp Outdated
size_t nLimitDescendantSize = GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000;
std::string errString;
if (!mempool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) {
strFailReason = _("Transaction has too long of a mempool chain");

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 2, 2016

Member

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...

@instagibbs instagibbs force-pushed the instagibbs:toolong branch 2 times, most recently Dec 2, 2016

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Dec 2, 2016

I also think this is good for backport.

src/wallet/wallet.cpp Outdated
@@ -2042,6 +2048,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs))
continue;

if (pcoin->GetMempoolAncestorCount() > nMaxAncestors)

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 2, 2016

Member

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

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

@instagibbs instagibbs force-pushed the instagibbs:toolong branch Dec 2, 2016

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2016

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 instagibbs force-pushed the instagibbs:toolong branch 2 times, most recently Dec 4, 2016

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

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

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Dec 5, 2016

@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.

src/wallet/wallet.cpp Outdated
@@ -2042,6 +2048,9 @@ bool CWallet::SelectCoinsMinConf(const CAmount& nTargetValue, int nConfMine, int
if (output.nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? nConfMine : nConfTheirs))
continue;

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 5, 2016

Member

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

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

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

src/wallet/wallet.cpp Outdated
SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 1, 1, 0, vCoins, setCoinsRet, nValueRet) ||
(bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nLimitAncestors/4, vCoins, setCoinsRet, nValueRet)) ||
(bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nLimitAncestors/2, vCoins, setCoinsRet, nValueRet)) ||
(bSpendZeroConfChange && SelectCoinsMinConf(nTargetValue - nValueFromPresetInputs, 0, 1, nLimitAncestors, vCoins, setCoinsRet, nValueRet));

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Dec 5, 2016

Member

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 instagibbs force-pushed the instagibbs:toolong branch Dec 5, 2016

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2016

@sdaftuar made sense to me, done

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 6, 2016

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?

@instagibbs instagibbs force-pushed the instagibbs:toolong branch to af9bedb Dec 13, 2016

@sipa

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

utACK 5882c09, with one nit.


bool CTxMemPool::TransactionWithinChainLimit(const uint256& txid, size_t chainLimit) const {
LOCK(cs);
if (exists(txid) && std::max(mapTx.find(txid)->GetCountWithAncestors(), mapTx.find(txid)->GetCountWithDescendants()) >= chainLimit)

This comment has been minimized.

Copy link
@sipa

sipa Dec 19, 2016

Member

You're doing 3 map lookups here. What about

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

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2016

fixed @sipa nit

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 19, 2016

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

@instagibbs instagibbs force-pushed the instagibbs:toolong branch Dec 19, 2016

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2016

@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.

@TheBlueMatt
Copy link
Contributor

left a comment

utACK ff79329ccfe3ee4387b9de72d82431c7b8132272 some comments but none worth delaying 0.13.2 for.

@@ -685,7 +687,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
* completion the coin set and corresponding actual target value is
* assembled
*/
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;
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;

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Dec 19, 2016

Contributor

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

@@ -2545,6 +2567,21 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
}
}

if (GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
// Lastly, ensure this tx will pass the mempool's chain limits

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Dec 19, 2016

Contributor

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.

if (GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) {
// Lastly, ensure this tx will pass the mempool's chain limits
LockPoints lp;
CTxMemPoolEntry entry(txNew, 0, 0, 0, 0, false, 0, false, 0, lp);

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Dec 19, 2016

Contributor

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.

src/txmempool.cpp Outdated
return true;
auto it = mapTx.find(txid);
return it == mapTx.end() || (it->GetCountWithAncestors() < chainLimit &&
it->GetCountWithDescendants() < chainLimit);

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Dec 20, 2016

Contributor

This really should be indented.

@instagibbs instagibbs force-pushed the instagibbs:toolong branch to cee1612 Dec 20, 2016

@laanwj laanwj merged commit cee1612 into bitcoin:master Dec 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Dec 20, 2016
Merge #9262: Prefer coins that have fewer ancestors, sanity check txn…
… before ATMP

cee1612 reduce number of lookups in TransactionWithinChainLimit (Gregory Sanders)
af9bedb Test for fix of txn chaining in wallet (Gregory Sanders)
5882c09 CreateTransaction: Don't return success with too-many-ancestor txn (Gregory Sanders)
0b2294a SelectCoinsMinConf: Prefer coins with fewer ancestors (Gregory Sanders)

@laanwj laanwj removed the Needs backport label Dec 20, 2016

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

Backported in #9382, removing label

@sipa sipa referenced this pull request Jan 10, 2017
16 of 18 tasks complete
codablock added a commit to codablock/dash that referenced this pull request Jan 18, 2018
Merge bitcoin#9262: Prefer coins that have fewer ancestors, sanity ch…
…eck txn before ATMP

cee1612 reduce number of lookups in TransactionWithinChainLimit (Gregory Sanders)
af9bedb Test for fix of txn chaining in wallet (Gregory Sanders)
5882c09 CreateTransaction: Don't return success with too-many-ancestor txn (Gregory Sanders)
0b2294a SelectCoinsMinConf: Prefer coins with fewer ancestors (Gregory Sanders)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#9262: Prefer coins that have fewer ancestors, sanity ch…
…eck txn before ATMP

cee1612 reduce number of lookups in TransactionWithinChainLimit (Gregory Sanders)
af9bedb Test for fix of txn chaining in wallet (Gregory Sanders)
5882c09 CreateTransaction: Don't return success with too-many-ancestor txn (Gregory Sanders)
0b2294a SelectCoinsMinConf: Prefer coins with fewer ancestors (Gregory Sanders)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
Merge bitcoin#9262: Prefer coins that have fewer ancestors, sanity ch…
…eck txn before ATMP

cee1612 reduce number of lookups in TransactionWithinChainLimit (Gregory Sanders)
af9bedb Test for fix of txn chaining in wallet (Gregory Sanders)
5882c09 CreateTransaction: Don't return success with too-many-ancestor txn (Gregory Sanders)
0b2294a SelectCoinsMinConf: Prefer coins with fewer ancestors (Gregory Sanders)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.