Dandelion TX's Never Submitted To Stempool & Relayed Into Stem Phase

Summary

After a fair amount of time digging into whether Dandelion is working correctly or not in DGB 8.22, I can confirm it is not. When dandelion is enabled and a TX is created it is never submitted to stempool & relayed. It goes straight to mempool and is visible to all nodes right away. This is confirmed by 2 test failures in the p2p_dandelion.py functional test as well as looking in the core code. It does appear that 8.22 will make connections to older 7.17.3 dandelion nodes and establish connections & embargoes from time to time. So it is partially working.

If you are brand new to Dandelion & want to learn more about what Dandelion is & how it works look here: https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki

Dandelion Functional Test

The functional test p2p_dandelion.py has three parts: tests 1 & 2 are currently failing while test 3 passes. https://github.com/DigiByte-Core/digibyte/blob/develop/test/functional/p2p_dandelion.py

Test 1: Resistance to active probing - checks that a tx is not visible to outside nodes on the initial creation of dandelion tx and initiation of stem phase.

Test 2: Loop behavior - checks that a tx is not visible to outside nodes later on in the stem phase.

Test 3: Resistance to black holes. Checks tx does eventually become visible in mempool after embargo and won't be lost forever in a black hole.

Tests 1 & 2 fail because a new tx goes straight to mempool, skipping stempool & is visible to all nodes right away.

To Run Test:

test/functional/test_runner.py p2p_dandelion.py

As I dove in I was led down a rabbit hole where I realized I had already been down the same rabbit hole a while back looking at issues with SubmitMemoryPoolAndRelay. It is clear I stumbled across this issue before with: https://github.com/DigiByte-Core/digibyte/issues/77

Where Dandelion Is Broken:

After digging deeper, it appears in 2019 this BTC pull request and changes is the crux of our dandelion problems. https://github.com/bitcoin/bitcoin/pull/15713/files

Basically, AcceptToMemoryPool and RelayWalletTransaction were combined to create SubmitMemoryPoolAndRelay. These changes inadvertently disabled our ability to submit new TX's to stempool and might be causing other issues with our implementation like relaying dandelion txs in stempool. Also, this is where dandelion needs to be enabled or disabled with DEFAULT_DANDELION

https://github.com/DigiByte-Core/digibyte/blob/99ed14a35cd31163a311ec98a96769b3c248443c/src/net.h#L82-L83

RelayWalletTransaction

Our 7.17.3 RelayWalletTransaction looks as follows: https://github.com/DigiByte-Core/digibyte/blob/develop/src/wallet/wallet.cpp#L1828

bool CWalletTx::RelayWalletTransaction(CConnman* connman)
{
    assert(pwallet->GetBroadcastTransactions());
    if (!IsCoinBase() && !isAbandoned() && GetDepthInMainChain() == 0)
    {
        LogPrintf("Inside IF Test \n");
        CValidationState state;
        /* GetDepthInMainChain already catches known conflicts. */
        LogPrintf("Transaction in memory pool: %u, Accepted To MemoryPool: %u", InMempool(), AcceptToMemoryPool(maxTxFee, state));
        if (InMempool() || AcceptToMemoryPool(maxTxFee, state)) {
            pwallet->WalletLogPrintf("Relaying wtx %s\n", GetHash().ToString());
            if (connman) {
                if (!gArgs.GetBoolArg("-disabledandelion", DEFAULT_DISABLE_DANDELION)) {
                    int64_t nCurrTime = GetTimeMicros();
                    int64_t nEmbargo = 1000000*DANDELION_EMBARGO_MINIMUM+PoissonNextSend(nCurrTime, DANDELION_EMBARGO_AVG_ADD);
                    connman->insertDandelionEmbargo(GetHash(),nEmbargo);
                    LogPrint(BCLog::DANDELION, "dandeliontx %s embargoed for %d seconds\n", GetHash().ToString(), (nEmbargo-nCurrTime)/1000000);
                    CInv inv(MSG_DANDELION_TX, GetHash());
                    return connman->localDandelionDestinationPushInventory(inv);
                } else {
                    CInv inv(MSG_TX, GetHash());
                    connman->ForEachNode([&inv](CNode* pnode)
                    {
                        pnode->PushInventory(inv);
                    });
                    return true;
                }
            }
        }
    }
    return false;
}

AcceptToMemoryPool

Where as our 7.17.3 AcceptToMemoryPool looks like: https://github.com/DigiByte-Core/digibyte/blob/develop/src/wallet/wallet.cpp#L4429

bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state)
{
    // We must set fInMempool here - while it will be re-set to true by the
    // entered-mempool callback, if we did not there would be a race where a
    // user could call sendmoney in a loop and hit spurious out of funds errors
    // because we think that this newly generated transaction's change is
    // unavailable as we're not yet aware that it is in the mempool.
    bool ret;
    if (!gArgs.GetBoolArg("-disabledandelion", DEFAULT_DISABLE_DANDELION)) {
        ret = ::AcceptToMemoryPool(stempool, state, tx, nullptr /* pfMissingInputs */,
                                   nullptr /* plTxnReplaced */, false /* bypass_limits */, nAbsurdFee);
    } else {
        ret = ::AcceptToMemoryPool(mempool, state, tx, nullptr /* pfMissingInputs */,
                                   nullptr /* plTxnReplaced */, false /* bypass_limits */, nAbsurdFee);
        // Changes to mempool should also be made to Dandelion stempool
        CValidationState dummyState;
        ret = ::AcceptToMemoryPool(stempool, dummyState, tx, nullptr /* pfMissingInputs */,
                                   nullptr /* plTxnReplaced */, false /* bypass_limits */, nAbsurdFee);
    }
    fInMempool |= ret;
    return ret;
}

SubmitMemoryPoolAndRelay

Our current v8.22.0 SubmitMemoryPoolAndRelay for sure needs rewritten to allow us to support & disable dandelion, and get relays functioning properly. This needs to allow for the initial submission of dandeliontx's to stempool and relay. https://github.com/DigiByte-Core/digibyte/blob/feature/8.22.0/src/wallet/wallet.cpp#L1712

bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay)
{
    // Can't relay if wallet is not broadcasting
    if (!pwallet->GetBroadcastTransactions()) return false;
    // Don't relay abandoned transactions
    if (isAbandoned()) return false;
    // Don't try to submit coinbase transactions. These would fail anyway but would
    // cause log spam.
    if (IsCoinBase()) return false;
    // Don't try to submit conflicted or confirmed transactions.
    if (GetDepthInMainChain() != 0) return false;

    // Submit transaction to mempool for relay
    pwallet->WalletLogPrintf("Submitting wtx %s to mempool for relay\n", GetHash().ToString());
    // We must set fInMempool here - while it will be re-set to true by the
    // entered-mempool callback, if we did not there would be a race where a
    // user could call sendmoney in a loop and hit spurious out of funds errors
    // because we think that this newly generated transaction's change is
    // unavailable as we're not yet aware that it is in the mempool.
    //
    // Irrespective of the failure reason, un-marking fInMempool
    // out-of-order is incorrect - it should be unmarked when
    // TransactionRemovedFromMempool fires.
    bool ret = pwallet->chain().broadcastTransaction(tx, pwallet->m_default_max_tx_fee, relay, err_string);
    fInMempool |= ret;
    return ret;
}

DigiByte-Core / digibyte