Package Relay Draft 1 (Single In-Flight Approach) #8

glozow · 2023-01-24T16:52:17Z

Experimental code.

General structure
This might be helpful for reviewing. The PR can also be split into sub-PRs like this.

Mempool logic to make sure we can handle whatever the peer throws at us (just rebased on here, can work on these in parallel). This PR is open, n26711.

(P2P stuff starts at commit 52bf734)
Refactoring, create a TxPackageTracker module that always exists even if package relay is turned off. It wraps the TxOrphanage and forwards calls.
Beef up an "orphan resolution module" within TxPackageTracker. It keeps track of what orphans we're trying to get parents for (currently still requesting by txid) and tells Peerman what should be downloaded/validated, when. Improve orphan handling by remembering which peers announced the orphan to allow retrying failed requests, requesting from preferred first, accounting for overloaded peers, etc. Tweak the eviction/rate limiting logic so that, even if an inbound is sending lots of orphans, we're still able to resolve orphans with outbounds at 1/2*INVENTORY_BROADCAST_PER_SECOND rate (for example).
Add a -packagerelay option and the "sendpackages" negotiation logic.
Add logic for requesting MSG_ANCPKGINFO and serving "ancpkginfo." When resolving orphans, prefer package relay peers and ask for "ancpkginfo" from them.
When receiving package info, track which transactions need to be downloaded. At this point, just download and validate individually. Protect stuff that's already in our orphanage from eviction. (Note: in the future, we can add new package info versions, while keeping the PackageToDownload data structures generic).
Add "getpkgtxns" and "pkgtxns" logic, and `MSG_PKGTXNS. Download and submit packages.

Todos

Have TxPackageTracker wrap/own TxOrphanage
Protect some quantity of orphans, ensure that we can always get packages relayed from outbounds
Tweak delays / expiry to make sure 1 peer can't keep stalling to make us forget tx
Needs unit tests and fuzz for TxPackageTracker
Cache rejections of subpackages

Done but in other branches

When Loading mempool from disk, ensure packages persist

Additional Maybe Todos

Look in vExtra for parents/transactions listed in ancpkginfo
if txn-already-have (recently confirmed), Exclude()
Add a map of <wtxid, {fees, size}> to MemPoolAccept to check ancestor subpackage fees prior to sending to AcceptMultipleTransactions
fuzz Packageifier
If orphanage is full, fall back to downloading another time

src/net_processing.cpp

mzumsande · 2023-02-10T20:24:07Z

src/net_processing.cpp

@@ -3259,6 +3277,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,

        if (greatest_common_version >= WTXID_RELAY_VERSION) {
            m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY));
+            if (!m_ignore_incoming_txs && m_enable_package_relay) {


also shouldn't send SENDPACKAGES for block-relay-only connections (check GetTxRelay()).

m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY));

this package would cost a block relay function to optimize it and render it. there should not bee the GetTxRelay()
function it would block the preview.
sets and manages terminal lines and ports

src/net_processing.cpp

dergoegge · 2023-01-24T17:30:10Z

src/txpackagerelay.cpp

+    {
+        AssertLockNotHeld(m_mutex);
+        LOCK(m_mutex);
+        const auto current_time{GetTime<std::chrono::seconds>()};


GetTime is deprecated

src/net_processing.cpp

src/txpackagerelay.h

dergoegge · 2023-03-03T16:16:51Z

src/txpackagerelay.cpp

+}
+class TxPackageTracker::Impl {
+    /** Manages unvalidated tx data (orphan transactions for which we are downloading ancestors). */
+    TxOrphanage& orphanage_ref;


all the members are missing the m_ prefix

src/txpackagerelay.cpp

src/txpackagerelay.h

glozow · 2023-03-03T16:34:43Z

Some notes on m_recent_rejects vs m_recent_rejects_reconsiderable.

The goals are:

Don't repeat work, i.e. don't download or validate transactions more than once if we can know they're going to be rejected.
Don't allow censorship due to overzealous rejection caching.

m_recent_rejects stores the wtxids of transactions we will not download/validate by themselves again until the chain tip changes. Even if we see them in a package, we'll reject them. This includes failures for consensus, and also some policy like tx-too-big.

m_recent_rejects_reconsiderable contains wtxids of individual transactions and packages that we will not download/validate by themselves, but will reconsider if they're part of a package. This includes transactions and packages that failed for being too low feerate. It can also include orphans and transactions with ephemeral anchor outputs where we haven't seen the child yet. The idea is that we'll reconsider them with another tx that makes them valid (e.g. high-fee child), but not otherwise.

Upon receiving an inv for 1 transaction, reject if it is in m_recent_rejects or m_recent_rejects_reconsiderable.

Upon receiving an ancpkginfo:

If any of the transactions are in m_recent_rejects, reject this package and don't download the txdata. However, it is ok to continue asking other peers for the ancpkginfo in case somebody else provides a different list.
If the entire package is in m_recent_rejects_reconsiderable, reject this package and don't download the txdata (it will just be duplicate work). However, it is ok to continue asking other peers for the ancpkginfo in case somebody else provides a different list.

Here is an example:

Let's take this order of events:

Downloaded orphan F. Resolved by downloading ancpkginfo {ABCDEF}
A: submit, {AC}
B: submit, B
C: submit, {AC}
D: reject
E: reject
F: reject
recent_rejects: D
reconsiderable: E, F, {ABCDEF}
You continue trying to resolve F. If another peer says the ancpkginfo is ABCDEF, there is no need to download/validate again.
Downloaded orphan G. Resolved by downloading ancpkginfo {EG}. E was rejected but is in the reconsiderable recent rejects, so you request the tx data and submit as a package.
E: reject
G: reject
reconsiderable: E, G, {EG}
You continue trying to resolve G. If another peer says the ancpkginfo is EG, there is no need to download/validate again.
Downloaded orphan H. Resolved by downloading ancpkginfo {EGH}. E and G and {EG} were rejected but are in the reconsiderable recent rejects, so you request the tx data and submit as a package.
E: submit, {EGH}
F: submit, {EGH}
H: submit, {EGH}
Downloaded orphan J. Resolved by downloading ancpkginfo {ABCDEFJ}.
You reject this package without downloading, because D had an invalid signature. However, you continue asking other peers for ancpkginfo in case the first peer lied to you. If they also say the ancpkginfo is {ABCDEFJ}, you don't download the tx data.

dergoegge · 2023-04-01T12:53:01Z

src/txorphanage.h

+    /** Protect an orphan from eviction from the orphanage getting full. The orphan may still be
+     * removed due to expiry. If the orphan is already protected (by any peer), nothing happens.
+     */
+    void ProtectOrphan(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);


Maybe add these new methods to fuzz/txorphan.cpp

src/node/txpackagetracker.h

dergoegge · 2023-04-01T13:02:44Z

src/node/txpackagetracker.h

+
+class CBlock;
+class TxOrphanage;
+


should wrap all of this in namespace node.

dergoegge · 2023-04-01T13:21:14Z

src/net_processing.cpp

+                if (ancestor_wtxids == std::nullopt) {
+                    pfrom.fDisconnect = true;
+                    // No need to process the other requests if we are disconnecting the peer.
+                    LogPrintf("Disconnecting peer %d -- requested ancpkginfo but not allowed\n", pfrom.GetId());


this LogPrintf looks like a disk filler

dergoegge · 2023-04-01T13:23:29Z

src/net_processing.cpp

+        if (package_wtxids.size() > MAX_PACKAGE_COUNT) {
+            LogPrint(BCLog::NET, "discarding package info for tx %s, too many transactions\n", rep_wtxid.ToString());
+            // FIXME: disconnect?
+            m_txpackagetracker->ForgetPkgInfo(pfrom.GetId(), package_wtxids.back(), RECEIVER_INIT_ANCESTOR_PACKAGES);
+            return;
+        }


would move the max size check up to the empty check

dergoegge · 2023-04-01T13:49:42Z

src/net_processing.cpp

+std::optional<std::vector<uint256>> PeerManagerImpl::MaybeGetAncPkgInfo(Peer& peer, const CTransactionRef& tx)
+{
+    if (!peer.m_package_relay) {
+        return std::nullopt;
+    }


Suggested change

std::optional<std::vector<uint256>> PeerManagerImpl::MaybeGetAncPkgInfo(Peer& peer, const CTransactionRef& tx)

{

if (!peer.m_package_relay) {

return std::nullopt;

}

std::vector<uint256> PeerManagerImpl::MaybeGetAncPkgInfo(const CTransactionRef& tx)

{

Check this at the call site of MaybeGetAncPkgInfo instead. Simplifies the function signature and you already have an if statement there for this anyway.

dergoegge · 2023-04-01T13:53:24Z

src/net_processing.cpp

+        if (!m_txpackagetracker) return;
+        std::vector<CTransactionRef> package_txns;
+        vRecv >> package_txns;
+        if (package_txns.empty()) return;


No size limit? Maybe do something similar to what we do when receiving headers (i.e. peek at the size before de-serializing)

dergoegge · 2023-04-01T13:56:18Z

src/net_processing.cpp

@@ -4616,6 +4948,51 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
        return;
    }

+    if (msg_type == NetMsgType::GETPKGTXNS) {
+        std::vector<uint256> txns_requested;
+        vRecv >> txns_requested;


No size limit?

dergoegge · 2023-04-01T14:02:18Z

src/net_processing.cpp

 {
    // While Erlay support is incomplete, it must be enabled explicitly via -txreconciliation.
    // This argument can go away after Erlay support is complete.
    if (gArgs.GetBoolArg("-txreconciliation", DEFAULT_TXRECONCILIATION_ENABLE)) {
        m_txreconciliation = std::make_unique<TxReconciliationTracker>(TXRECONCILIATION_VERSION);
    }
+    m_txpackagetracker = std::make_unique<TxPackageTracker>(m_orphanage);


Suggested change

m_txpackagetracker = std::make_unique<TxPackageTracker>(m_orphanage);

if (m_enable_package_relay) m_txpackagetracker = std::make_unique<TxPackageTracker>(m_orphanage);

Or if the package tracker should always exist, make it a non-unique_ptr member of PeerManagerImpl. Then you can also remove all the existence checks along the way.

src/net_processing.cpp

jnewbery · 2023-04-03T18:03:35Z

src/node/txpackagetracker.cpp

+        bool m_txrelay{true};
+        // Whether this peer sent a BIP339 wtxidrelay message.
+        bool m_wtxid_relay{false};


These two members are only used when they're written in ReceivedVerack() and then immediately read back. Can you remove them and have the CanRelayPackages() logic directly in ReceivedVerack()?

jnewbery · 2023-04-03T18:05:11Z

src/node/txpackagetracker.cpp

+        // Whether this peer sent a BIP339 wtxidrelay message.
+        bool m_wtxid_relay{false};
+        /** Whether this peer says they can do package relay. */
+        bool m_sendpackages_received{false};


Can we remove this bool and replace it with !m_versions_in_common.empty()? Do we care if the peer has only sent us sendpackages for versions we don't understand?

jnewbery · 2023-04-03T18:05:51Z

src/node/txpackagetracker.cpp

+
+public:
+    Impl(TxOrphanage& orphanage) : orphanage_ref{orphanage} {}
+    void ReceivedVersion(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)


Do we need this function? Can we just add the node to registration_states in ReceivedSendpackages() if it isn't there already?

Yeah, tbh it seems like I overcomplicated this and could just have 1 function to add a peer after verack

jnewbery · 2023-04-03T18:06:34Z

src/node/txpackagetracker.cpp

+
+    struct PeerInfo {
+        // What package versions we agreed to relay.
+        std::set<uint32_t> m_versions_supported;


Can we make this const and set it in the constructor?

jnewbery · 2023-04-03T18:10:18Z

src/node/txpackagetracker.cpp

+    /** Manages unvalidated tx data (orphan transactions for which we are downloading ancestors). */
+    TxOrphanage& orphanage_ref;
+
+    mutable Mutex m_mutex;


Perhaps:

Suggested change

mutable Mutex m_mutex;

mutable Mutex m_package_tracker_mutex;

to help with grepping.

src/node/txpackagetracker.cpp

jnewbery · 2023-04-03T18:14:55Z

src/net_processing.cpp

@@ -768,6 +786,9 @@ class PeerManagerImpl final : public PeerManager
    /** Number of peers with wtxid relay. */
    std::atomic<int> m_wtxid_relay_peers{0};

+    /** Number of peers with package relay. */
+    std::atomic<int> m_package_relay_peers{0};


Can this (and Peer:m_package_relay) be removed and we just query m_txpackagetracker for whether a peer supports package relay and how many peers we have that support package relay?

Avoid adding transactions below min relay feerate because, even if they were bumped through CPFP when entering the mempool, we do not have a DoS-resistant way of ensuring they always remain bumped. In the future, this rule can be relaxed (e.g. to allow packages to bump 0-fee transactions) if we find a way to do so.

Avoid calling PackageMempoolChecks() when there is only 1 transaction. Note to reviewers: there is a slight change in the error type returned, as shown in the txpackage_tests change. When a transaction is the last one left in the package and its fee is too low, this returns a PCKG_TX instead of PCKG_POLICY. This interface is clearer; "package-fee-too-low" for 1 transaction would be a bit misleading.

At this point it's not expected that there are any such transactions, except from reorgs.

The intention of the test is to set a high -minrelaytxfee such that these transactions are rejected in LoadMempool() and in CWallet::ResumbitWalletTransactions(). However, while the parent transactions are below minrelaytxfee, they each have descendants high enough feerate to bump them past the minrelaytxfee (observe that the `assert_greater_than` checks fail if given the original amount of 0.0001). These transactions will be kept after the mempool persists packages, which is an improvement, but will cause the original test to fail.

Hold pool.cs the entire time otherwise wallet resubmissions may call TrimtoSize() in between loading transactions from disk.

-BEGIN VERIFY SCRIPT- sed -i 's/CheckPackage(/IsPackageWellFormed(/g' $(git grep -l CheckPackage) -END VERIFY SCRIPT-

We cannot require that peers send topologically sorted lists, because we cannot check for this property without ensuring we have the same chain tip and ensuring we have the full ancestor set. Instead, add the ability to handle arbitrarily ordered transaction lists.

Packageifier calculates the in-package ancestors. We already know this tx will fail because it depends on something invalid (specifically for a non-policy reason). We also know that it is missing at least one input (the tx that did not and will not make it into our mempool). Just return missing inputs directly. Note to reviewers: slight behavior change. If this tx has multiple errors, there may be a difference in which one is returned. For example, if the tx has a dust output in addition to relying on the invalid tx, we will return TX_MISSING_INPUTS when we previously would have returned TX_NOT_STANDARD. This is simply because dust is checked earlier within PreChecks(). Add a test that we don't quit *too* early and reject transactions we should keep.

…ackages This results in the incentive-compatible transactions ending up in our mempool. Prior to this commit, if parents within the package relied on each other, we could end up (1) accepting a low-feerate child or (2) rejecting high-feerate parents. Instead of validating each transaction *individually* in turn, validate each one with their in-package ancestor set. This means parents with inter-dependencies are validated correctly, while we continue using aggregate totals for package feerate.

…ow fees We wouldn't want to add a low-feerate transaction to m_recent_rejects because usually that means we won't give the child a chance (`fRejectedParents`); we instead want to fetch this orphan's low-feerate parents and potentially accept them. However, we also shouldn't revalidate transactions/packages that have already been rejected. Continue to cache low-fee rejections, but separately. Note: when ephemeral anchor transaction fails due to a missing spender, we should similarly put it in the reconsiderable filter.

Conditions for responding to an ancpkginfo request are identical to responding to a tx. That is, we only provide ancpkginfo if we would have provided tx data as well. (HACKY AND UNSAFE)

… tracker

For now, each tx in ancpkginfo is treated like an individual tx announcement. When the tx arrives, it is validated individually as well.

FIXME: packagehash for python tests

These transactions can be more common with package relay, but non-package-relay peers will only waste bandwidth and orphanage space, then end up not accepting them. Save their bandwidth by dropping announcements of transactions that have below-fee-filter parents.

ariard

Reviewed up to “[validation] set PackageValidationState when mempool full”.

ariard · 2023-05-01T01:06:15Z

doc/policy/packages.md

+5sat/vB and a 1sat/vB parent transaction has a high-feerate child, it may be accepted if
+submitted as a package.
+
+*Rationale*: This can be thought of as "CPFP within a package," solving the issue of a presigned


I think this new “CPFP within a package” ability might alter pinning considerations on legacy channels (i.e before option_anchor_outputs is still the default on CLN iirc and at least on LDK).

The to_remote output format on legacy commitment transaction is a P2WPKH. There is no 1 CSV as with anchor output. As soon as the local commitment transaction is broadcast on the transaction-relay network, the counterparty can attach a CPFP to amplify the propagation probabilities of the local commitment transaction. This CPFP attachment might be not be seen by the broadcaster of the local commitment transaction. This CPFP can be used as a pinning child vector (e.g rule 3 on absolute fee). The broadcast of a local commitment transaction can be be trigger by a malicious counterparty by refusing an update_fee even in the lack of pending HTLC outputs.

So from my understanding the deployment of a “CPFP within a package” might enable some new pinning scenarios against legacy channels (even when there is no pending HTLC outputs) ? I think in term of severity those scenarios are equivalent where the pinnings are trigger by a counterparty and there are pending HTLC outputs. Note there is no currently standardized or deplyed channel upgrade mechanism to uplift legacy channel types to newer anchor output.

Independently of pinnings, I wonder if those low-feerate commitment transactions carried on by a “CPFP within a package” could be leveraged to open the way to some miner fee-harvesting attacks ?

We might have to swallow the bullet after all. Though if we have do it, I prefer to do it in knowledge of the opening attack surface, at least if we can land some easy mitigations on the Lightning-side.

ariard · 2023-05-01T01:36:21Z

doc/policy/packages.md

+transaction (i.e. in which a replacement transaction with a higher fee cannot be signed) being
+rejected from the mempool when transaction volume is high and the mempool minimum feerate rises.
+
+Note: Package feerate cannot be used to meet the minimum relay feerate (`-minrelaytxfee`)


What is the state of the discussion about modifying or removing -minrelaytxfee ? I think there was some discussions in the past as tracked by optech https://bitcoinops.org/en/topics/default-minimum-transaction-relay-feerates/

From my understanding, minrelaytxfee constitutes a strong transaction-relay propagation liability for contract protocols as mentioned in mempool-limits.md. I think if you have a pre-signed 1sat/vbyte LN commitment transaction or a vault withdrawal transaction 5sat/vbyte and there is a major increase in the minrelaytxfee to 10sat/vbyte all your transactions won’t propagate, and this whatever the CPFP feerate iiuc proposed changes.

With current -minrelaytxfee of 1 sat/vbyte (DEFAULT_INCREMENTAL_RELAY_FEE) this not a concrete propagation risk for contract protocol. However this setting might be changed at the initiative of node operators or by the project itself in the future. From my understanding, this would put contract protocol node operators in a difficult position as LN counterparty cooperation cannot be assumed to re-sign new transactions to a higher feerate or vault key ceremony coordinated before there is a deployment of such “bumped” -minrelaytxfee.

So I’m thinking if a “wildcard” could be granted to nversion=3 transactions where the parent transaction can be any feeerate and this compensated by the CPFP ?

I don't think we are planning to remove -minrelaytxfee entirely.

Yes, the plan is to allow v3 transactions to be 0 fee when they are cpfp'd

ariard · 2023-05-01T01:53:04Z

src/validation.cpp

-    // No individual transactions are allowed below the min relay feerate and mempool min feerate except from
-    // disconnected blocks and transactions in a package. Package transactions will be checked using
-    // package feerate later.
+    // No individual transactions are allowed below the min relay feerate except from disconnected blocks.


What is the pruning strategy in the hypothesis of disconnected blocks where low-feerate package transactions are resuscitated back in the network mempools and the CPFP replaced following this disconnection by a better block mining a partial double-spend of the CPFP but not the parent themselves ?

ariard · 2023-05-01T02:03:16Z

src/validation.cpp

@@ -1200,6 +1200,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector<Workspace>&
        } else {
            all_submitted = false;
            ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full");
+            package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");


I think it’s a bit confusing that a PackageValidationResult’s reject_reason says transaction failed. A more indicative error message could be whole package submission failed - individual transaction mempool inclusion failed. It’s more verbose though at least make easier to debug that the package integrity has been respected by our mempool logic.

ariard

Reviewed until “[mempool] evict everything below min relay fee in TrimToSize()"

ariard · 2023-05-02T02:57:52Z

src/validation.cpp

+        AssertLockHeld(::cs_main);
+        AssertLockHeld(m_pool.cs);
+        if (subpackage.size() > 1) {
+            return AcceptMultipleTransactions(subpackage, args);


Note IsPackageWellFormed is called twice for the same set of transaction, once L1349 and once L1249. From my understanding of the checks there, and with the fact that only a diminished of transactions is given the second time, the checks should always yield valid. A bypass argument could be given to AcceptSingleTransaction to gain a bit in performance.

A more fundamental simplication could be to unify submitpackage and testmempoolaccept. I don’t know if we have gathered users feedback on the relevance of multi-transactions testmempoolaccept since it’s available. At the very least the difference in code paths and as such of checks can be a source of confusion as much for developers than for users on what properties are enforced.

ariard · 2023-05-02T03:12:12Z

src/txmempool.cpp

+        CFeeRate removed(it->GetModFeesWithDescendants(), it->GetSizeWithDescendants());
+        // Skim away everything paying effectively zero fees, regardless of mempool size.
+        // Above that feerate, just trim until memory is within limits.
+        if (removed >= m_min_relay_feerate && DynamicMemoryUsage() <= sizelimit) break;


I think this creates a non-incentive-compatible optimal blind spot where a transaction feerate could be under m_min_relay_fee though with a non-zero absolute fee and network mempools being empty of better blockspace demand. All that said, I’m not sure such transactions can be issued by robust broadcast clients respecting m_min_relay_feerate, there is still the possibility of wrongly implemented clients or relying on third-party fee-estimators for such checks ? I don’t see other risks.

f952e67 ci: remove usage of untrusted bpfcc-tools (fanquake) 1232c2f ci: use LLVM/clang-16 in native_asan job (fanquake) Pull request description: Similar to bitcoin#27298. Working for me on `x86_64` and solves the issue I currently see with TSAN on `aarch64` with master (6882828): ```bash crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0xffff84400406 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment 0xffff84400406: note: pointer points here b9 c5 22 00 01 01 1a 6c 65 76 65 6c 64 62 2e 42 79 74 65 77 69 73 65 43 6f 6d 70 61 72 61 74 6f ^ #0 0xaaaaaddaf0b4 in crc32c::ExtendArm64(unsigned int, unsigned char const*, unsigned long) src/./src/crc32c/src/crc32c_arm64.cc:101:26 #1 0xaaaaadd2c838 in leveldb::crc32c::Value(char const*, unsigned long) src/./leveldb/util/crc32c.h:20:60 #2 0xaaaaadd2c838 in leveldb::log::Reader::ReadPhysicalRecord(leveldb::Slice*) src/./src/leveldb/db/log_reader.cc:246:29 #3 0xaaaaadd2ba9c in leveldb::log::Reader::ReadRecord(leveldb::Slice*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*) src/./src/leveldb/db/log_reader.cc:72:38 #4 0xaaaaadd41710 in leveldb::VersionSet::Recover(bool*) src/./src/leveldb/db/version_set.cc:910:19 #5 0xaaaaadcf9fec in leveldb::DBImpl::Recover(leveldb::VersionEdit*, bool*) src/./src/leveldb/db/db_impl.cc:320:18 #6 0xaaaaadd12068 in leveldb::DB::Open(leveldb::Options const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, leveldb::DB**) src/./src/leveldb/db/db_impl.cc:1487:20 #7 0xaaaaad314e80 in CDBWrapper::CDBWrapper(DBParams const&) src/./src/dbwrapper.cpp:156:30 #8 0xaaaaace94880 in CBlockTreeDB::CBlockTreeDB(DBParams const&) src/./txdb.h:89:23 #9 0xaaaaace94880 in std::_MakeUniq<CBlockTreeDB>::__single_object std::make_unique<CBlockTreeDB, DBParams>(DBParams&&) /usr/bin/../lib/gcc/aarch64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:962:34 bitcoin#10 0xaaaaace94880 in ChainTestingSetup::ChainTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) src/./src/test/util/setup_common.cpp:188:51 bitcoin#11 0xaaaaace95da0 in TestingSetup::TestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&, bool, bool) src/./src/test/util/setup_common.cpp:243:7 bitcoin#12 0xaaaaace96730 in TestChain100Setup::TestChain100Setup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&, bool, bool) src/./src/test/util/setup_common.cpp:274:7 bitcoin#13 0xaaaaac1ddbc8 in blockfilter_index_tests::BuildChainTestingSetup::BuildChainTestingSetup() src/./src/test/blockfilter_index_tests.cpp:26:8 bitcoin#14 0xaaaaac1ddbc8 in blockfilter_index_tests::blockfilter_index_initial_sync::blockfilter_index_initial_sync() src/./src/test/blockfilter_index_tests.cpp:112:1 bitcoin#15 0xaaaaac1ddbc8 in blockfilter_index_tests::blockfilter_index_initial_sync_invoker() src/./src/test/blockfilter_index_tests.cpp:112:1 bitcoin#16 0xaaaaabf08f7c in boost::function0<void>::operator()() const /usr/include/boost/function/function_template.hpp:763:14 bitcoin#17 0xaaaaabf95468 in boost::detail::forward::operator()() /usr/include/boost/test/impl/execution_monitor.ipp:1388:32 bitcoin#18 0xaaaaabf95468 in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:137:18 bitcoin#19 0xaaaaabf8e12c in boost::function0<int>::operator()() const /usr/include/boost/function/function_template.hpp:763:14 bitcoin#20 0xaaaaabe7be14 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:903:16 bitcoin#21 0xaaaaabe7c1c0 in boost::execution_monitor::execute(boost::function<int ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1301:16 bitcoin#22 0xaaaaabe6f47c in boost::execution_monitor::vexecute(boost::function<void ()> const&) /usr/include/boost/test/impl/execution_monitor.ipp:1397:5 bitcoin#23 0xaaaaabe75124 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /usr/include/boost/test/impl/unit_test_monitor.ipp:49:9 bitcoin#24 0xaaaaabed19fc in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:815:44 bitcoin#25 0xaaaaabed0f6c in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58 bitcoin#26 0xaaaaabed0f6c in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /usr/include/boost/test/impl/framework.ipp:784:58 bitcoin#27 0xaaaaabe73878 in boost::unit_test::framework::run(unsigned long, bool) /usr/include/boost/test/impl/framework.ipp:1721:29 bitcoin#28 0xaaaaabe9d244 in boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /usr/include/boost/test/impl/unit_test_main.ipp:250:9 bitcoin#29 0xffff8f0773f8 (/lib/aarch64-linux-gnu/libc.so.6+0x273f8) (BuildId: f37f3aa07c797e333fd106472898d361f71798f5) bitcoin#30 0xffff8f0774c8 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x274c8) (BuildId: f37f3aa07c797e333fd106472898d361f71798f5) bitcoin#31 0xaaaaabda55ac in _start (/home/fedora/ci_scratch/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/test_bitcoin+0x10e55ac) (BuildId: b7909adaefd9db6cd6a7c4d3d40207cf6bdaf4b3) SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use crc32c/src/crc32c_arm64.cc:101:26 in ``` ACKs for top commit: dergoegge: utACK f952e67 MarcoFalke: lgtm ACK f952e67 Tree-SHA512: 9dee2abf73d3f23bb9979bfb453b48e39f0b7a5f58d43824ecf053a53e9800ed413b915382b274d1a84baf2999683e3b485463e377e0455b3f0ead65ed1d1916

ariard · 2023-05-07T16:23:37Z

src/kernel/mempool_persist.cpp

+                // this means fee-bumped transactions are persisted, and the resulting mempool
+                // minimum feerate is not dependent on the order in which transactions are loaded
+                // from disk.
+                const auto& accepted = AcceptToMemoryPool(active_chainstate, tx, nTime, /*bypass_limits=*/true, /*test_accept=*/false);


From my understanding, if you shutdown at block H with mempool size S and dump mempool file M, then if you re-load mempool file M at same block H and with same configuration of mempool size S, the whole content of the file should be loaded in memory with no transaction marked as evicted ? If we would like to improve error detection, like a mempool file corrupted, maybe we can “watermark” the mempool file with some information like block height, mempool size at time of dump, configured mempool min fees…if it’s not already the case ? I’m not sure if error detection of mempool dump is that important, though it might be annoying for a miner to have a stale mempool view.

ariard · 2023-05-07T16:44:11Z

src/policy/packages.h

+/** If any direct dependencies exist between transactions (i.e. a child spending the output of a
+ * parent), checks that all parents appear somewhere in the list before their respective children.
+ * This function cannot detect indirect dependencies (e.g. a transaction's grandparent if its parent
+ * is not present).


Can say there is no max bound enforce and “sort” verification can happen on any size and depth of package?

682274a ci: install llvm-symbolizer in MSAN jobs (fanquake) 96527cd ci: use LLVM 16.0.6 in MSAN jobs (fanquake) Pull request description: Fixes: bitcoin#27737 (comment). Tested (locally) with bitcoin#27495 that it produces a symbolized backtrace: ```bash 2023-06-20T17:5Uninitialized bytes in __interceptor_strlen at offset 113 inside [0x719000006908, 114) ==35429==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x56060fae8c4b in sqlite3Strlen30 /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:32670:28 #1 0x56060fb0fcf4 in sqlite3PagerOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:57953:17 #2 0x56060fb0f48b in sqlite3BtreeOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:68679:10 #3 0x56060fb01384 in openDatabase /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:171911:8 #4 0x56060fb016ca in sqlite3_open_v2 /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:172034:10 #5 0x56060e8a94db in wallet::SQLiteDatabase::Open() src/wallet/sqlite.cpp:250:19 #6 0x56060e8a30fd in wallet::SQLiteDatabase::SQLiteDatabase(fs::path const&, fs::path const&, wallet::DatabaseOptions const&, bool) src/wallet/sqlite.cpp:133:9 #7 0x56060e8b78f5 in std::__1::__unique_if<wallet::SQLiteDatabase>::__unique_single std::__1::make_unique[abi:v160006]<wallet::SQLiteDatabase, std::__1::__fs::filesystem::path, fs::path&, wallet::DatabaseOptions const&>(std::__1::__fs::filesystem::path&&, fs::path&, wallet::DatabaseOptions const&) /home/ubuntu/ci_scratch/ci/scratch/msan/cxx_build/include/c++/v1/__memory/unique_ptr.h:686:30 #8 0x56060e8b5240 in wallet::MakeSQLiteDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/sqlite.cpp:641:19 #9 0x56060e83560b in wallet::MakeDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/walletdb.cpp:1261:16 bitcoin#10 0x56060e7546e9 in wallet::MakeWalletDatabase(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/wallet.cpp:2905:12 bitcoin#11 0x56060e4bc03f in wallet::TestLoadWallet(wallet::WalletContext&) src/wallet/test/util.cpp:68:21 bitcoin#12 0x56060e349ad4 in wallet::wallet_tests::ZapSelectTx::test_method() src/wallet/test/wallet_tests.cpp:897:19 bitcoin#13 0x56060e348598 in wallet::wallet_tests::ZapSelectTx_invoker() src/wallet/test/wallet_tests.cpp:891:1 bitcoin#14 0x56060cfec325 in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 bitcoin#15 0x56060ced3a7e in boost::function0<void>::operator()() const /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14 bitcoin#16 0x56060ced3a7e in boost::detail::forward::operator()() /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1388:32 bitcoin#17 0x56060ced3a7e in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:137:18 bitcoin#18 0x56060cda71c2 in boost::function0<int>::operator()() const /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14 bitcoin#19 0x56060cda71c2 in int boost::detail::do_invoke<boost::shared_ptr<boost::detail::translator_holder_base>, boost::function<int ()>>(boost::shared_ptr<boost::detail::translator_holder_base> const&, boost::function<int ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:301:30 bitcoin#20 0x56060cda71c2 in boost::execution_monitor::catch_signals(boost::function<int ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:903:16 bitcoin#21 0x56060cda784a in boost::execution_monitor::execute(boost::function<int ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1301:16 bitcoin#22 0x56060cd9ec3a in boost::execution_monitor::vexecute(boost::function<void ()> const&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1397:5 bitcoin#23 0x56060cd9ec3a in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned long) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_monitor.ipp:49:9 bitcoin#24 0x56060ce1a07b in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:815:44 bitcoin#25 0x56060ce1ad8b in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:784:58 bitcoin#26 0x56060ce1ad8b in boost::unit_test::framework::state::execute_test_tree(unsigned long, unsigned long, boost::unit_test::framework::state::random_generator_helper const*) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:784:58 bitcoin#27 0x56060cd9b8de in boost::unit_test::framework::run(unsigned long, bool) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/framework.ipp:1722:29 bitcoin#28 0x56060cdd4fac in boost::unit_test::unit_test_main(boost::unit_test::test_suite* (*)(int, char**), int, char**) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:250:9 bitcoin#29 0x56060cdd6094 in main /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/unit_test_main.ipp:306:12 bitcoin#30 0x7f7379691d8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) bitcoin#31 0x7f7379691e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d) bitcoin#32 0x56060cce2e24 in _start (/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/test_bitcoin+0x188e24) Uninitialized value was created by a heap allocation #0 0x56060cd163f2 in malloc /ci_base_install/ci/scratch/msan/llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:934:3 #1 0x56060fc10069 in sqlite3MemMalloc /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:25163:7 #2 0x56060fb063bc in mallocWithAlarm /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:28846:7 #3 0x56060fae4eb9 in sqlite3Malloc /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:28876:5 #4 0x56060faf9e19 in sqlite3DbMallocRaw /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:29176:7 #5 0x56060fb0fc67 in sqlite3PagerOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:57938:17 #6 0x56060fb0f48b in sqlite3BtreeOpen /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:68679:10 #7 0x56060fb01384 in openDatabase /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:171911:8 #8 0x56060fb016ca in sqlite3_open_v2 /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:172034:10 #9 0x56060e8a94db in wallet::SQLiteDatabase::Open() src/wallet/sqlite.cpp:250:19 bitcoin#10 0x56060e8a30fd in wallet::SQLiteDatabase::SQLiteDatabase(fs::path const&, fs::path const&, wallet::DatabaseOptions const&, bool) src/wallet/sqlite.cpp:133:9 bitcoin#11 0x56060e8b78f5 in std::__1::__unique_if<wallet::SQLiteDatabase>::__unique_single std::__1::make_unique[abi:v160006]<wallet::SQLiteDatabase, std::__1::__fs::filesystem::path, fs::path&, wallet::DatabaseOptions const&>(std::__1::__fs::filesystem::path&&, fs::path&, wallet::DatabaseOptions const&) /home/ubuntu/ci_scratch/ci/scratch/msan/cxx_build/include/c++/v1/__memory/unique_ptr.h:686:30 bitcoin#12 0x56060e8b5240 in wallet::MakeSQLiteDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/sqlite.cpp:641:19 bitcoin#13 0x56060e83560b in wallet::MakeDatabase(fs::path const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/walletdb.cpp:1261:16 bitcoin#14 0x56060e7546e9 in wallet::MakeWalletDatabase(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, wallet::DatabaseOptions const&, wallet::DatabaseStatus&, bilingual_str&) src/wallet/wallet.cpp:2905:12 bitcoin#15 0x56060e4bc03f in wallet::TestLoadWallet(wallet::WalletContext&) src/wallet/test/util.cpp:68:21 bitcoin#16 0x56060e349ad4 in wallet::wallet_tests::ZapSelectTx::test_method() src/wallet/test/wallet_tests.cpp:897:19 bitcoin#17 0x56060e348598 in wallet::wallet_tests::ZapSelectTx_invoker() src/wallet/test/wallet_tests.cpp:891:1 bitcoin#18 0x56060cfec325 in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:117:11 bitcoin#19 0x56060ced3a7e in boost::function0<void>::operator()() const /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:763:14 bitcoin#20 0x56060ced3a7e in boost::detail::forward::operator()() /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1388:32 bitcoin#21 0x56060ced3a7e in boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) /home/ubuntu/ci_scratch/depends/x86_64-pc-linux-gnu/include/boost/function/function_template.hpp:137:18 SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ubuntu/ci_scratch/depends/work/build/x86_64-pc-linux-gnu/sqlite/3380500-f816a3e2d52/sqlite3.c:32670:28 in sqlite3Strlen30 ``` as opposed to unsymbolized: https://cirrus-ci.com/task/6005512018329600?logs=ci#L3245. ACKs for top commit: MarcoFalke: lgtm ACK 682274a Tree-SHA512: 8f3e7636761c956537a472989bf07529f5afbd988c5e7e1f07ece8b2599608fa4fe9e1efdc6e302cf0f7f44dec3cf9a3c1e68b758af81a8a8b476a43d3220807

…BlockTx suppression fa9dc92 test: Add missing CBlockPolicyEstimator::processBlockTx suppression (MarcoFalke) Pull request description: Fixes bitcoin#28865 (comment) ``` # FUZZ=policy_estimator UBSAN_OPTIONS="suppressions=/root/fuzz_dir/scratch/fuzz_gen/code/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 ... policy/fees.cpp:632:27: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed) #0 0x55cbbe10daee in CBlockPolicyEstimator::processBlockTx(unsigned int, CTxMemPoolEntry const*) src/policy/fees.cpp:632:27 #1 0x55cbbe10e361 in CBlockPolicyEstimator::processBlock(unsigned int, std::vector<CTxMemPoolEntry const*, std::allocator<CTxMemPoolEntry const*>>&) src/policy/fees.cpp:680:13 #2 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>)::$_1::operator()() const src/test/fuzz/policy_estimator.cpp:69:40 #3 0x55cbbd84af48 in unsigned long CallOneOf<policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3>(FuzzedDataProvider&, policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3) src/./test/fuzz/util.h:43:27 #4 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>) src/test/fuzz/policy_estimator.cpp:38:9 #5 0x55cbbda1cc18 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #6 0x55cbbda1cc18 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #7 0x55cbbd26a944 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x190e944) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #8 0x55cbbd253916 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f7916) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #9 0x55cbbd25945a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18fd45a) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) bitcoin#10 0x55cbbd284026 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x1928026) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) bitcoin#11 0x7fe4aa8280cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) bitcoin#12 0x7fe4aa828188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) bitcoin#13 0x55cbbd24e494 in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f2494) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change policy/fees.cpp:632:27 in ``` ``` # base64 /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 AQEAAAAAADkFlVwAAQEAAAAAADkFlZVcACTDSSsP3746IAZrH48khwMAAQEB/QEALQAACwAAAAAA FgAAAAAAAQAABgAAAAAAAAAAAAAAAAAAACcQAAAAAAAAAAAAAAAAAAAAAAD6AAAAOQWVXAABAQAA AAAAOQWVlVwAIMNJKw/fvjogBmsfjySHAwABAQH9AQAtAAALAAAAAAAAAAABAAAGAAAAAAAAAAAA AAAAAAAAJxAAAAAAAAAAAAAAAAAAAAAAAPr/AAAAAAAAAAAAAAQAAAAA/wAAAAAAAAAAAAAEAAAA AAEBAeAIAVwBXAAA/jbSBvwBKABSKBwBYgEB2wAEkvXInHYAAAAAAAAAvgAAAAAA/9//6v8e/xIk MgAlAiUAOw== ACKs for top commit: fanquake: ACK fa9dc92 dergoegge: utACK fa9dc92 Tree-SHA512: 3898c17c928ecc2bcc8c7086359e9ae00da2197b4d8e10c7bf6d12415326c9bca3ef6e1d8d3b83172ccfa604ce7e7371415262ba705225f9ea4da8b1a7eb0306

…tifications fuzz target fab164f fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target (MarcoFalke) Pull request description: Should avoid ``` policy/feerate.cpp:29:63: runtime error: signed integer overflow: 77600710321911316 * 149 cannot be represented in type 'int64_t' (aka 'long') #0 0x563a1775ed66 in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63 #1 0x563a15913a69 in wallet::COutput::COutput(COutPoint const&, CTxOut const&, int, int, bool, bool, bool, long, bool, std::optional<CFeeRate>) src/./wallet/coinselection.h:91:57 #2 0x563a16fa6a6d in wallet::FetchSelectedInputs(wallet::CWallet const&, wallet::CCoinControl const&, wallet::CoinSelectionParams const&) src/wallet/spend.cpp:297:17 #3 0x563a16fc4512 in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1105:33 #4 0x563a16fbec74 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1291:16 #5 0x563a16fcf6df in wallet::FundTransaction(wallet::CWallet&, CMutableTransaction&, long&, int&, bilingual_str&, bool, std::set<int, std::less<int>, std::allocator<int>> const&, wallet::CCoinControl) src/wallet/spend.cpp:1361:16 #6 0x563a1597b7b9 in wallet::(anonymous namespace)::FuzzedWallet::FundTx(FuzzedDataProvider&, CMutableTransaction) src/wallet/test/fuzz/notifications.cpp:162:15 #7 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0::operator()() const src/wallet/test/fuzz/notifications.cpp:228:23 #8 0x563a15958240 in unsigned long CallOneOf<wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1>(FuzzedDataProvider&, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1) src/./test/fuzz/util.h:43:27 #9 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/notifications.cpp:196:9 bitcoin#10 0x563a15fdef0c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 bitcoin#11 0x563a15fdef0c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 bitcoin#12 0x563a158032a4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19822a4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#13 0x563a15802999 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1981999) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#14 0x563a15804586 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983586) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#15 0x563a15804aa7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983aa7) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#16 0x563a157f21fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19711fb) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#17 0x563a1581c766 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199b766) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#18 0x7f499e17b0cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) bitcoin#19 0x7f499e17b188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) bitcoin#20 0x563a157e70c4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19660c4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in MS: 0 ; base unit: 0000000000000000000000000000000000000000 0x3f,0x0,0x2f,0x5f,0x5f,0x5f,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0xff,0xff,0xff,0xff,0xff,0x53,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x13,0x5e,0x5f,0x5f,0x8,0x25,0x0,0x5f,0x5f,0x5f,0x5f,0x5f,0x5f,0x8,0x25,0xca,0x7f,0x5f,0x5f,0x5f,0x13,0x13,0x5f,0x5f,0x5f,0x2,0xdb,0xca,0x0,0x0,0xe7,0xe6,0x66,0x65,0x0,0x0,0x0,0x0,0x44,0x3f,0xa,0xa,0xff,0xff,0xff,0xff,0xff,0x61,0x76,0x6f,0x69,0x0,0xb5,0x15, ?\000/___}}}}}}}}}}}}}}}}}}}}\377\377\377\377\377S\377\377\377\377\377\000\000\000\000\000\000\023^__\010%\000______\010%\312\177___\023\023___\002\333\312\000\000\347\346fe\000\000\000\000D?\012\012\377\377\377\377\377avoi\000\265\025 artifact_prefix='./'; Test unit written to ./crash-4d3bac8a64d4e58b2f0943e6d28e6e1f16328d7d Base64: PwAvX19ffX19fX19fX19fX19fX19fX19fX3//////1P//////wAAAAAAABNeX18IJQBfX19fX18IJcp/X19fExNfX18C28oAAOfmZmUAAAAARD8KCv//////YXZvaQC1FQ== ACKs for top commit: dergoegge: ACK fab164f brunoerg: ACK fab164f Tree-SHA512: f416828f4394aa7303ee437f141e9bbd23c0e0f1b830e4ef3932338858249ba68a811b9837c5b7ad8c6ab871b6354996434183597c1a910a8d8e8d829693e4b2

The previous commit added a test which would fail the unsigned-integer-overflow sanitizer. The warning is harmless and can be triggered on any commit, since the code was introduced. For reference, the warning would happen when the separator `-` was not present. For example: GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json would result in: rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long') #0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77 #1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9 #3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13 #4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12 #5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2) #6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o #7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) #8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3) SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77

glozow force-pushed the 2023-01-package-relay branch 3 times, most recently from 1b0f313 to d41dfba Compare February 3, 2023 08:52

mzumsande reviewed Feb 10, 2023

View reviewed changes

glozow force-pushed the 2023-01-package-relay branch 2 times, most recently from 3d40ff7 to 913044a Compare February 24, 2023 11:34

dergoegge reviewed Mar 3, 2023

View reviewed changes

src/txpackagerelay.h Outdated Show resolved Hide resolved

glozow force-pushed the 2023-01-package-relay branch from 913044a to 397063c Compare March 6, 2023 17:03

glozow force-pushed the 2023-01-package-relay branch 2 times, most recently from 14fad0b to 5ef9545 Compare March 22, 2023 13:15

dergoegge reviewed Apr 1, 2023

View reviewed changes

jnewbery reviewed Apr 3, 2023

View reviewed changes

glozow added 16 commits April 18, 2023 10:10

-- Part 0: Mempool Logic for Arbitrary Packages--

43fd360

[test util] mock mempool minimum feerate

f154c34

[test] package cpfp bumps parents <mempoolminfee but >=minrelaytxfee

5d09d97

[validation] set PackageValidationState when mempool full

1f40f8c

[mempool] evict everything below min relay fee in TrimToSize()

a781945

At this point it's not expected that there are any such transactions, except from reorgs.

[test framework] return txhex from create_lots_of_big_transactions

daf6b81

[mempool] persist packages across restart

faf8b23

Hold pool.cs the entire time otherwise wallet resubmissions may call TrimtoSize() in between loading transactions from disk.

MOVEONLY: move package checks into helper functions

f9b1993

scripted-diff: rename CheckPackage to IsPackageWellFormed

2d0d4c0

-BEGIN VERIFY SCRIPT- sed -i 's/CheckPackage(/IsPackageWellFormed(/g' $(git grep -l CheckPackage) -END VERIFY SCRIPT-

[test util] multi-input multi-output CreateValidMempoolTransaction

87e43a5

glozow added 16 commits April 19, 2023 12:43

[packages] hash multiple transactions

797ed82

[p2p] respond to getdata(ancpkginfo) requests with ancpkginfo

8164ad2

Conditions for responding to an ancpkginfo request are identical to responding to a tx. That is, we only provide ancpkginfo if we would have provided tx data as well. (HACKY AND UNSAFE)

[txpackagetracker] create ancpkginfo requests using orphan resolution…

e6656a9

… tracker

[unit test] txpackagetracker orphan tracking

0389892

[p2p] Resolve orphans by requesting ancpkginfo and requesting txdata

21acc89

For now, each tx in ancpkginfo is treated like an individual tx announcement. When the tx arrives, it is validated individually as well.

-- Part 4: Download and Validate Packages --

cc800b3

[p2p] respond to getpkgtxns with pkgtxns or notfound MSG_PKGTXNS

ba5e401

FIXME: packagehash for python tests

[txpackagetracker] track packages to download

852da8e

[p2p] use ancpkginfo to make getpkgtxns requests

fbe58b5

[txpackagetracker] handle pkgtxns/notfound, return validation work items

8859429

[unit test] PkgInfoAllowed, ReceivedAncPkgInfo, ReceivedPkgTxns

252ff5a

[p2p] handle pkgtxns and validate packages

54c4fbe

[rpc] allow submitpackage to be called outside of regtest

d682a10

[functional test] package relay e2e

b647166

glozow mentioned this pull request Apr 20, 2023

Package Relay Draft 2 (Orphanage Eviction Approach) #9

Closed

glozow force-pushed the 2023-01-package-relay branch from 5ef9545 to b647166 Compare April 22, 2023 13:41

ariard reviewed May 1, 2023

View reviewed changes

ariard mentioned this pull request May 1, 2023

Package Relay Project Tracking bitcoin/bitcoin#27463

Open

54 tasks

ariard reviewed May 2, 2023

View reviewed changes

ariard reviewed May 7, 2023

View reviewed changes

glozow changed the title ~~WIP Package Relay Draft 1~~ Package Relay Draft 1 (Single In-Flight Approach) Jun 1, 2023

glozow mentioned this pull request Jun 14, 2023

BIP 331: Ancestor Package Relay bitcoin/bips#1382

Merged

glozow closed this Nov 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package Relay Draft 1 (Single In-Flight Approach) #8

Package Relay Draft 1 (Single In-Flight Approach) #8

glozow commented Jan 24, 2023 •

edited

Loading

mzumsande Feb 10, 2023

Yugthakkar04 Sep 18, 2023

dergoegge Jan 24, 2023

dergoegge Mar 3, 2023

glozow commented Mar 3, 2023 •

edited

Loading

dergoegge Apr 1, 2023

dergoegge Apr 1, 2023

dergoegge Apr 1, 2023

dergoegge Apr 1, 2023

dergoegge Apr 1, 2023

dergoegge Apr 1, 2023

dergoegge Apr 1, 2023

dergoegge Apr 1, 2023

jnewbery Apr 3, 2023

jnewbery Apr 3, 2023

jnewbery Apr 3, 2023

glozow Apr 20, 2023

jnewbery Apr 3, 2023

jnewbery Apr 3, 2023

jnewbery Apr 3, 2023

ariard left a comment

ariard May 1, 2023

ariard May 1, 2023

glozow Jun 1, 2023

ariard May 1, 2023

ariard May 1, 2023

ariard left a comment

ariard May 2, 2023

ariard May 2, 2023

ariard May 7, 2023

ariard May 7, 2023

	m_txpackagetracker = std::make_unique<TxPackageTracker>(m_orphanage);
	if (m_enable_package_relay) m_txpackagetracker = std::make_unique<TxPackageTracker>(m_orphanage);

	mutable Mutex m_mutex;
	mutable Mutex m_package_tracker_mutex;

Package Relay Draft 1 (Single In-Flight Approach) #8

Package Relay Draft 1 (Single In-Flight Approach) #8

Conversation

glozow commented Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow commented Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow commented Jan 24, 2023 •

edited

Loading

glozow commented Mar 3, 2023 •

edited

Loading