WIP: Feature/8.22.0 Random Crash Bug & Missing Dandelion Relay TX Code about digibyte

[JaredTate]

Barry, appreciate you helping take a look at this. I see you are using gdb to debug, which reminded me of something good to share with the community.

I wanted to share a debugging guide that was original created for Bitcoin but is very applicable to DGB. Depending on wether you are on Linux or Mac this guide is geared towards Mac. Basically its important to enable debugging both during compiling as well as while running DigiByted / DigiByte-Qt. Typically once you get an indicator of where an error might be by looking at the debug.log in the default DGB directory then you can take a deeper look with lldb(on mac) by setting break points. Or taking a look with valgrind at memory leaks or enabling core dumps on macOS.
However without having a general idea of where to look from debug.log valgrind & lldb can spit out a lot of unhelpful info to sort through.

It is useful to run these tools with the unit tests and functional tests. Which is even more of a reason we need to make sure all tests are updated. Idea is to step through the problems to try and debug, which can be challenging depending on whats going on. You can see this guide here: https://gist.github.com/fjahr/2cd23ad743a2ddfd4eed957274beca0f

[JaredTate]

After digging deeper, it appears in 2019 this BTC pull request and changes are the crux of this problem I believe. https://github.com/bitcoin/bitcoin/pull/15713/files

Basically AcceptToMemoryPool and RelayWalletTransaction were combined to create SubmitMemoryPoolAndRelay. These changes inadvertently disabled our ability to disable dandelion at all, and might be causing other issues with our implementation and relaying dandelion txs, as well as other txs. There was a lot of relay rework done, but the above BTC PR is a good place to find where to look and see where else things might be breaking as well.

-disabledandelion & DEFAULT_DISABLE_DANDELION are completely missing from any part of the current 8.22.0 code.

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;
}

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;
}

Our current v8.22.0 SubmitMemoryPoolAndRelay for sure needs rewritten to allow us to support & disable dandelion, and get relays functioning properly. 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;
}

[barrystyle]

Have just noticed (https://github.com/DigiByte-Core/digibyte/blame/feature/8.22.0/src/net_processing.cpp#L2005) which appears to remove the given tx hash from mempool - then immediately afterwards is iterating an array searching for that tx hash (https://github.com/DigiByte-Core/digibyte/blame/feature/8.22.0/src/net_processing.cpp#L2012).

[barrystyle]

@JaredTate
check node/transaction.cpp which is where the majority of the mempool submit code got moved to.
i've also noticed that the txid/wtxid of a given transaction are quite often the same; which isn't correct (there are actually assert conditions throughout the code if this behaviour is seen).

[barrystyle]

[JaredTate]

After diving in more I have 100% been able to confirm this is a transaction relay bug. There is an option in the wallet to add on startup or in the digibyte.conf called "blocksonly" which also enables "disablebroadcast" which prevents the client from relaying any transactions and only relaying completed blocks. By adding blocksonly=1 to the digibyte.conf on all 4 mining algos on DigiHash the client has not crashed and has been stable for almost 24 hours now, where as they usually crashed within an hour. Same with my local client. This confirms its a tx relay issue.

I have also ran a full LLDB frame backtrace which provided some additional insight.

(lldb) up
frame #1: 0x000000010121d3f6 digibyted`SipHashUint256(k0=17938932625010577665, k1=10144771775799500642, val=0x0000000000000038) at siphash.cpp:97:22
   94  	uint64_t SipHashUint256(uint64_t k0, uint64_t k1, const uint256& val)
   95  	{
   96  	    /* Specialized implementation for efficiency */
-> 97  	    uint64_t d = val.GetUint64(0);
   98  	
   99  	    uint64_t v0 = 0x736f6d6570736575ULL ^ k0;
   100 	    uint64_t v1 = 0x646f72616e646f6dULL ^ k1;
(lldb) up
frame #2: 0x000000010090a286 digibyted`SaltedTxidHasher::operator(this=0x00000001123d0838, txid=0x0000000000000038)(uint256 const&) const at hasher.h:22:16
   19  	    SaltedTxidHasher();
   20  	
   21  	    size_t operator()(const uint256& txid) const {
-> 22  	        return SipHashUint256(k0, k1, txid);
   23  	    }
   24  	};
   25  	
(lldb) up
frame #3: 0x0000000100549bd0 digibyted`boost::multi_index::detail::hashed_index_iterator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::__1::allocator<CTxMemPoolEntry> > > > > > >, boost::multi_index::detail::bucket_array<std::__1::allocator<CTxMemPoolEntry> >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag> boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::__1::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher, mpl_::na, mpl_::na>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mempoolentry_wtxid, SaltedTxidHasher, mpl_::na>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, std::__1::allocator<CTxMemPoolEntry> >, boost::mpl::vector0<mpl_::na>, boost::multi_index::detail::hashed_unique_tag>::find<uint256, SaltedTxidHasher, std::__1::equal_to<uint256> >(this=0x00000001123d07e0, k=0x0000000000000038, hash=0x00000001123d0838, eq=0x00000001123d0848, (null)=mpl_::false_ @ 0x00007000101571b8) const at hashed_index.hpp:1605:38
   1602	    const CompatibleKey& k,
   1603	    const CompatibleHash& hash,const CompatiblePred& eq,mpl::false_)const
   1604	  {
-> 1605	    std::size_t buc=buckets.position(hash(k));
   1606	    for(node_impl_pointer x=buckets.at(buc)->prior();
   1607	        x!=node_impl_pointer(0);x=node_alg::next_to_inspect(x)){
   1608	      if(eq(k,key(index_node_type::from_impl(x)->value()))){
(lldb) up
frame #4: 0x0000000100549b3f digibyted`boost::multi_index::detail::hashed_index_iterator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::__1::allocator<CTxMemPoolEntry> > > > > > >, boost::multi_index::detail::bucket_array<std::__1::allocator<CTxMemPoolEntry> >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag> boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::__1::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher, mpl_::na, mpl_::na>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mempoolentry_wtxid, SaltedTxidHasher, mpl_::na>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, std::__1::allocator<CTxMemPoolEntry> >, boost::mpl::vector0<mpl_::na>, boost::multi_index::detail::hashed_unique_tag>::find<uint256, SaltedTxidHasher, std::__1::equal_to<uint256> >(this=0x00000001123d07e0, k=0x0000000000000038, hash=0x00000001123d0838, eq=0x00000001123d0848) const at hashed_index.hpp:541:12
   538 	    const CompatibleKey& k,
   539 	    const CompatibleHash& hash,const CompatiblePred& eq)const
   540 	  {
-> 541 	    return find(
   542 	      k,hash,eq,promotes_1st_arg<CompatiblePred,CompatibleKey,key_type>());
   543 	  }
   544 	
(lldb) up
frame #5: 0x0000000100547b11 digibyted`boost::multi_index::detail::hashed_index_iterator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::__1::allocator<CTxMemPoolEntry> > > > > > >, boost::multi_index::detail::bucket_array<std::__1::allocator<CTxMemPoolEntry> >, boost::multi_index::detail::hashed_unique_tag, boost::multi_index::detail::hashed_index_global_iterator_tag> boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::__1::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher, mpl_::na, mpl_::na>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, mempoolentry_wtxid, SaltedTxidHasher, mpl_::na>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na, mpl_::na>, std::__1::allocator<CTxMemPoolEntry> >, boost::mpl::vector0<mpl_::na>, boost::multi_index::detail::hashed_unique_tag>::find<uint256>(this=0x00000001123d07e0, k=0x0000000000000038) const at hashed_index.hpp:531:12
   528 	  template<typename CompatibleKey>
   529 	  iterator find(const CompatibleKey& k)const
   530 	  {
-> 531 	    return find(k,hash_,eq_);
   532 	  }
   533 	
   534 	  template<
(lldb) up
frame #6: 0x000000010084fe37 digibyted`CTxMemPool::GetIter(this=0x00000001123d0730, txid=0x0000000000000038) const at txmempool.cpp:903:21
   900 	
   901 	std::optional<CTxMemPool::txiter> CTxMemPool::GetIter(const uint256& txid) const
   902 	{
-> 903 	    auto it = mapTx.find(txid);
   904 	    if (it != mapTx.end()) return it;
   905 	    return std::nullopt;
   906 	}
(lldb) up
frame #7: 0x000000010034ae36 digibyted`(anonymous namespace)::PeerManagerImpl::ProcessGetData(this=0x00000001123d1480, pfrom=0x0000000106022e00, peer=0x00000002c5ffb658, interruptMsgProc=0x0000000105020fc8)::Peer&, std::__1::atomic<bool> const&) at net_processing.cpp:2012:41
   2009	            std::vector<uint256> parent_ids_to_add;
   2010	            {
   2011	                LOCK(m_mempool.cs);
-> 2012	                auto txiter = m_mempool.GetIter(tx->GetHash());
   2013	                if (txiter) {
   2014	                    const CTxMemPoolEntry::Parents& parents = (*txiter)->GetMemPoolParentsConst();
   2015	                    parent_ids_to_add.reserve(parents.size());
(lldb) lup
error: 'lup' is not a valid command.
(lldb) up
frame #8: 0x00000001002e9265 digibyted`(anonymous namespace)::PeerManagerImpl::ProcessMessage(this=0x00000001123d1480, pfrom=0x0000000106022e00, msg_type="getdata", vRecv=0x00000002ca3e4910, time_received=(__rep_ = 1635283113509758), interruptMsgProc=0x0000000105020fc8) at net_processing.cpp:3076:13
   3073	        {
   3074	            LOCK(peer->m_getdata_requests_mutex);
   3075	            peer->m_getdata_requests.insert(peer->m_getdata_requests.end(), vInv.begin(), vInv.end());
-> 3076	            ProcessGetData(pfrom, *peer, interruptMsgProc);
   3077	        }
   3078	
   3079	        return;
(lldb) up
frame #9: 0x00000001002f3f90 digibyted`(anonymous namespace)::PeerManagerImpl::ProcessMessages(this=0x00000001123d1480, pfrom=0x0000000106022e00, interruptMsgProc=0x0000000105020fc8) at net_processing.cpp:4232:9
   4229	    unsigned int nMessageSize = msg.m_message_size;
   4230	
   4231	    try {
-> 4232	        ProcessMessage(*pfrom, msg_type, msg.m_recv, msg.m_time, interruptMsgProc);
   4233	        if (interruptMsgProc) return false;
   4234	        {
   4235	            LOCK(peer->m_getdata_requests_mutex);
(lldb) up
frame #10: 0x00000001002f9880 digibyted`non-virtual thunk to (anonymous namespace)::PeerManagerImpl::ProcessMessages(CNode*, std::__1::atomic<bool>&) at net_processing.cpp:0
   1   	// Copyright (c) 2009-2010 Satoshi Nakamoto
   2   	// Copyright (c) 2009-2020 The Bitcoin Core developers
   3   	// Copyright (c) 2014-2020 The DigiByte Core developers
   4   	// Distributed under the MIT software license, see the accompanying
   5   	// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   6   	
   7   	#include <net_processing.h>
(lldb) up
frame #11: 0x0000000100228d0d digibyted`CConnman::ThreadMessageHandler(this=0x0000000105020c00) at net.cpp:2574:45
   2571	                continue;
   2572	
   2573	            // Receive messages
-> 2574	            bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc);
   2575	            fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
   2576	            if (flagInterruptMsgProc)
   2577	                return;
(lldb) up
frame #12: 0x00000001002ba616 digibyted`CConnman::Start(this=0x000070001015bee8)::$_15::operator()() const at net.cpp:2923:80
   2920	    }
   2921	
   2922	    // Process messages
-> 2923	    threadMessageHandler = std::thread(&util::TraceThread, "msghand", [this] { ThreadMessageHandler(); });
   2924	
   2925	    if (connOptions.m_i2p_accept_incoming && m_i2p_sam_session.get() != nullptr) {    
   2926	        threadI2PAcceptIncoming =
(lldb) up
frame #13: 0x00000001002ba57b digibyted`decltype(__f=0x000070001015bee8)::$_15&>(fp)()) std::__1::__invoke<CConnman::Start(CScheduler&, CConnman::Options const&)::$_15&>(CConnman::Start(CScheduler&, CConnman::Options const&)::$_15&) at type_traits:3747:1
   3744	inline _LIBCPP_INLINE_VISIBILITY
   3745	auto
   3746	__invoke(_Fp&& __f, _Args&& ...__args)
-> 3747	_LIBCPP_INVOKE_RETURN(_VSTD::forward<_Fp>(__f)(_VSTD::forward<_Args>(__args)...))
   3748	
   3749	template <class _Fp, class ..._Args>
   3750	inline _LIBCPP_INLINE_VISIBILITY
(lldb) up
frame #14: 0x00000001002ba4cb digibyted`void std::__1::__invoke_void_return_wrapper<void>::__call<CConnman::Start(__args=0x000070001015bee8)::$_15&>(CConnman::Start(CScheduler&, CConnman::Options const&)::$_15&) at __functional_base:348:9
   345 	#ifndef _LIBCPP_CXX03_LANG
   346 	    template <class ..._Args>
   347 	    static void __call(_Args&&... __args) {
-> 348 	        __invoke(_VSTD::forward<_Args>(__args)...);
   349 	    }
   350 	#else
   351 	    template <class _Fn>
(lldb) up
frame #15: 0x00000001002ba47b digibyted`std::__1::__function::__alloc_func<CConnman::Start(CScheduler&, CConnman::Options const&)::$_15, std::__1::allocator<CConnman::Start(CScheduler&, CConnman::Options const&)::$_15>, void ()>::operator(this=0x000070001015bee8)() at functional:1553:16
   1550	    _Rp operator()(_ArgTypes&&... __arg)
   1551	    {
   1552	        typedef __invoke_void_return_wrapper<_Rp> _Invoker;
-> 1553	        return _Invoker::__call(__f_.first(),
   1554	                                _VSTD::forward<_ArgTypes>(__arg)...);
   1555	    }
   1556	
(lldb) up
frame #16: 0x00000001002b7f30 digibyted`std::__1::__function::__func<CConnman::Start(CScheduler&, CConnman::Options const&)::$_15, std::__1::allocator<CConnman::Start(CScheduler&, CConnman::Options const&)::$_15>, void ()>::operator(this=0x000070001015bee0)() at functional:1727:12
   1724	_Rp
   1725	__func<_Fp, _Alloc, _Rp(_ArgTypes...)>::operator()(_ArgTypes&& ... __arg)
   1726	{
-> 1727	    return __f_(_VSTD::forward<_ArgTypes>(__arg)...);
   1728	}
   1729	
   1730	#ifndef _LIBCPP_NO_RTTI
(lldb) up
frame #17: 0x000000010049afe3 digibyted`std::__1::__function::__value_func<void ()>::operator(this=0x000070001015bee0)() const at functional:1880:16
   1877	    {
   1878	        if (__f_ == 0)
   1879	            __throw_bad_function_call();
-> 1880	        return (*__f_)(_VSTD::forward<_ArgTypes>(__args)...);
   1881	    }
   1882	
   1883	    _LIBCPP_INLINE_VISIBILITY
(lldb) up
frame #18: 0x000000010049af77 digibyted`std::__1::function<void ()>::operator(this=0x000070001015bee0)() const at functional:2555:12
   2552	_Rp
   2553	function<_Rp(_ArgTypes...)>::operator()(_ArgTypes... __arg) const
   2554	{
-> 2555	    return __f_(_VSTD::forward<_ArgTypes>(__arg)...);
   2556	}
   2557	
   2558	#ifndef _LIBCPP_NO_RTTI

[JaredTate]

After re-reading the Dandelion BIP a couple other things have stood out to me as well: https://github.com/bitcoin/bips/blob/master/bip-0156.mediawiki

Because the Embargo & Dandelion relay code to set an embargo is completely missing in SubmitMemoryPoolAndRelay, the wallet has no idea what to do with stempool transactions during the fluff phase, and the possibility exists they are never relaying and completely stall out. Meaning the dandelion TX would completely fail without adding an embargo. The embargo code is needed to ensure that if a client suddenly goes off line, after the embargo time expires all previous clients in the stem will go ahead and broadcast the tx as normal tx as their embargoes expire.

So this if not fixed right, it could cause dandelion TX's to completely fail without the embargo code.

During the stem phase, transactions are relayed along a single path. 
If any node in this path were to receive the Dandelion transaction and go offline, then the transaction would cease to propagate. 
To increase robustness, every node that forwards a Dandelion transaction initializes a timer at the time of reception.
 If the Dandelion transaction does not appear in the memory pool by the time the timer expires, 
 then the transaction enters fluff phase and is forwarded via diffusion.

When a Dandelion transaction arrives, the node MUST set an embargo timer for a random time in the future. 
If the Dandelion transaction arrives as a typical Bitcoin transaction, the node MUST cancel the timer. 
If the timer expires before the Dandelion transaction is observed as a typical Bitcoin transaction, 
then the node MUST fluff the Dandelion transaction.

So the code below from RelayWalletTransaction above is not only needed for disabling dandelion, but to provide the needed embargos for dandelion transactions. This needs to be reworked into SubmitMemoryPoolAndRelay. However that is proving more challenging that expected because of the BTC core rework of 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 {

[JaredTate]

Barry, good find. That would explain the the bad access error.

(this=0x00000001123d07e0, k=0x0000000000000038) const at hashed_index.hpp:531:12
   528 	  template<typename CompatibleKey>
   529 	  iterator find(const CompatibleKey& k)const
   530 	  {
-> 531 	    return find(k,hash_,eq_);
   532 	  }
   533 	
   534 	  template<
(lldb) up
frame #6: 0x000000010084fe37 digibyted`CTxMemPool::GetIter(this=0x00000001123d0730, txid=0x0000000000000038) const at txmempool.cpp:903:21
   900 	
   901 	std::optional<CTxMemPool::txiter> CTxMemPool::GetIter(const uint256& txid) const
   902 	{
-> 903 	    auto it = mapTx.find(txid);
   904 	    if (it != mapTx.end()) return it;
   905 	    return std::nullopt;
   906 	}
(lldb) up
frame #7: 0x000000010034ae36 digibyted`(anonymous namespace)::PeerManagerImpl::ProcessGetData(this=0x00000001123d1480, pfrom=0x0000000106022e00, peer=0x00000002c5ffb658, interruptMsgProc=0x0000000105020fc8)::Peer&, std::__1::atomic<bool> const&) at net_processing.cpp:2012:41
   2009	            std::vector<uint256> parent_ids_to_add;
   2010	            {
   2011	                LOCK(m_mempool.cs);
-> 2012	                auto txiter = m_mempool.GetIter(tx->GetHash());
   2013	                if (txiter) {
   2014	                    const CTxMemPoolEntry::Parents& parents = (*txiter)->GetMemPoolParentsConst();
   2015	                    parent_ids_to_add.reserve(parents.size());
(lldb) lup
error: 'lup' is not a valid command.
(lldb) up
frame #8: 0x00000001002e9265 digibyted`(anonymous namespace)::PeerManagerImpl::ProcessMessage(this=0x00000001123d1480, pfrom=0x0000000106022e00, msg_type="getdata", vRecv=0x00000002ca3e4910, time_received=(__rep_ = 1635283113509758), interruptMsgProc=0x0000000105020fc8) at net_processing.cpp:3076:13
   3073	        {
   3074	            LOCK(peer->m_getdata_requests_mutex);
   3075	            peer->m_getdata_requests.insert(peer->m_getdata_requests.end(), vInv.begin(), vInv.end());
-> 3076	            ProcessGetData(pfrom, *peer, interruptMsgProc);
   3077	        }
   3078	
   3079	        return;
(lldb) up
frame #9: 0x00000001002f3f90 digibyted`(anonymous namespace)::PeerManagerImpl::ProcessMessages(this=0x00000001123d1480, pfrom=0x0000000106022e00, interruptMsgProc=0x0000000105020fc8) at net_processing.cpp:4232:9
   4229	    unsigned int nMessageSize = msg.m_message_size;
   4230	
   4231	    try {
-> 4232	        ProcessMessage(*pfrom, msg_type, msg.m_recv, msg.m_time, interruptMsgProc);
   4233	        if (interruptMsgProc) return false;
   4234	        {
   4235	            LOCK(peer->m_getdata_requests_mutex);
(lldb) up

Also on a side note about my comment above, even with blocksonly enabled in digibyte.conf, people can still add forcerelay to their conf and client will still try to relay.

[JaredTate]

I also want to point out by reverting this commit: d351fd0

Everything else seems to be stable and work in 8.22 now that I can test, except for dandelion. When I merged this PR, I compiled, tested and sent some txs. But I never let the client run long enough to hit this bug.

So if we are not able to get this sorted in the next week, we could take the dandelion work out and create a new feature branch just focused on 8.22 dandelion as there are a few things that need updated and reworked I believe. Then carry on testing everything else in 8.22 without dandelion.

There is definitely a reason all the BTC devs abandoned adding dandelion in 0.20, .21 and v22. There is a lot of rework done with mempool and tx relays, so this is quite a dynamic problem to solve.

I am confident we all can get through this and get dandelion working as expected before a full 8.22 release though.

When we get through this, there is a chance we could even structure a PR and submit a working dandelion to BTC core and get some collaboration going with BTC devs.

[barrystyle]

I also want to point out by reverting this commit: d351fd0

Everything else seems to be stable and work in 8.22 now that I can test, except for dandelion. When I merged this PR, I compiled, tested and sent some txs. But I never let the client run long enough to hit this bug.

So if we are not able to get this sorted in the next week, we could take the dandelion work out and create a new feature branch just focused on 8.22 dandelion as there are a few things that need updated and reworked I believe. Then carry on testing everything else in 8.22 without dandelion.

There is definitely a reason all the BTC devs abandoned adding dandelion in 0.20, .21 and v22. There is a lot of rework done with mempool and tx relays, so this is quite a dynamic problem to solve.

I am confident we all can get through this and get dandelion working as expected before a full 8.22 release though.

When we get through this, there is a chance we could even structure a PR and submit a working dandelion to BTC core and get some collaboration going with BTC devs.

I feel the mempool stuff has not been migrated across correctly in the merge, as it should strictly be a function accessible via *node, not referenced as a global. Additionally quite a few asserts are missing on the main functions in validation.cpp, which in themselves cause compilation issues under clang (theyve been removed?).

I will submit another PR with the change i've suggested.

[JaredTate]

All help is much appreciated Barry. Thanks for taking a crack at this. This is a challenging problem.

There has been a massive amount of mempool & p2p rework done in BTC core since 0.17 as well as other things critical to original dandelion implementation. (We are effectively jumping from BTC 0.17 to BTC v22... so all BTC changes up to that point are to be dealt with to get a properly working dandelion client).

Reading through the BTC v22 release notes again and I can see several changes in net_processing that might be the root of some of these issues.
https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-22.0.md

This one is a biggy:
bitcoin/bitcoin#21162 Net Processing: Move RelayTransaction() into PeerManager (jnewbery)

As is this one:
bitcoin/bitcoin#21160 #net/net processing: Move tx inventory into net_processing

Others include:
bitcoin/bitcoin#20972 Locks: Annotate CTxMemPool::check to require cs_main (dongcarl)
bitcoin/bitcoin#22415 Make m_mempool optional in CChainState (jamesob)
bitcoin/bitcoin#19771 Replace enum CConnMan::NumConnections with enum class ConnectionDirection (luke-jr)
bitcoin/bitcoin#20162 p2p: declare Announcement::m_state as uint8_t, add getter/setter (jonatack)
bitcoin/bitcoin#21186 net/net processing: Move addr data into net_processing (jnewbery)
bitcoin/bitcoin#21187 Net processing: Only call PushAddress() from net_processing (jnewbery)
bitcoin/bitcoin#22013 ignore block-relay-only peers when skipping DNS seed (ajtowns)
bitcoin/bitcoin#22261 Two small fixes to node broadcast logic (jnewbery)

This one definitely caused some compiling issues in dandelion:
bitcoin/bitcoin#21015 Make all of net_processing (and some of net) use std::chrono types (dhruv)

Its apparent for sure part of dandelion needs rewritten and tweaked to function properly with all these BTC changes.