backport: merge bitcoin#19940, #19498, #19339, #19753, #20581, #20834, #21062, #21783, #21121, #22084, partial bitcoin#19668, #20833 (mempool backports) #5834

kwvg · 2024-01-18T19:59:15Z

Additional Information

Dependency for backport: merge bitcoin#20332, #20908, #21336, #21443, #21142, #21512, #20833, #21522, #21553, #21970, #24336, partial bitcoin#21606 (fuzzing harness backports: part 7) #5835

knst · 2024-01-25T22:02:45Z

src/rpc/rawtransaction.cpp

@@ -1072,6 +1072,11 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
                        {
                            {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
                            {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"},
+                            {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141"},


should we just drop vsize from output because that's equal to regular size of tx in 100% cases?
That has meaning only for case of segregated tx when 'real' and 'virtual' size of tx are different

I've dropped the "as defined in BIP 141" portion of the description but have retained vsize as getrawmempool already prints out vsize and a bunch of functional tests already refer to it like mempool_updatefromblock and mining_prioritisetransaction.

It's practically just an alias to size at this point but as long as somewhere something tries to fetch vsize, it's better there than not. Though on the other hand some RPCs might have already purged vsize since I don't see them outside of the examples I've mentioned so maybe there's merit to removing them.

Regardless, it'd have to be done in a distinct commit since we'd have to regularize other vsize usage.

github-actions · 2024-01-28T04:57:50Z

This pull request has conflicts, please rebase.

test/functional/test_framework/messages.py

src/validation.h

src/net_processing.cpp

knst · 2024-01-29T21:50:05Z

src/test/miner_tests.cpp

@@ -35,7 +35,8 @@ struct MinerTestingSetup : public TestingSetup {
    void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs);
    bool TestSequenceLocks(const CTransaction& tx, int flags) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs)
    {
-        return CheckSequenceLocks(::ChainstateActive(), *m_node.mempool, tx, flags);
+        CCoinsViewMemPool viewMempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool);


20833 - missing changes in src/test/fuzz/tx_pool.cpp

bitcoin#20833 has been listed as a partial and the commit description explicitly states the exclusion of c9e1a26, which would be completed in dash#5835 after the PR that introduces tx_pool (bitcoin#21142, backported as 6823ba3) has been backported. The completed bitcoin#20833 can be seen in 12c534e as part of dash#5835.

UdjinM6

LGTM, utACK

src/coinjoin/coinjoin.cpp

knst · 2024-02-01T17:54:40Z

src/coinjoin/coinjoin.cpp

@@ -306,6 +306,25 @@ bool CCoinJoinBaseSession::IsValidInOuts(const CTxMemPool& mempool, const std::v
    return true;
 }

+// Responsibility for checking fee sanity is moved from the mempool to the client (BroadcastTransaction)
+// but CoinJoin still requires ATMP with fee sanity checks so we need to implement them separately


why we can't drop it?

CoinJoin code expects that ATMP will check fee sanity because unlike every other non-RPC/non-GUI AcceptToMemoryPool call which uses an nAbsurdFee value of 0 (to skip absurd fee checking), CoinJoin code in a few places uses DEFAULT_MAX_RAW_TX_FEE (not to be confused with DEFAULT_MAX_RAW_TX_FEE_RATE, which has the same value, except as CFeeRate as opposed to CAmount) as nAbsurdFee, triggering the fee check.

Perhaps it could be refactored to be removed entirely but that's outside the scope of the PR and the backport attempts to preserve functionality as-is on the CoinJoin front.

UdjinM6

re-utACK

knst

utACK

PastaPastaPasta

utACK for merging via merge commit

Co-authored-by: Konstantin Akimov <[email protected]>

…clients

Co-authored-by: UdjinM6 <[email protected]>

…ut" parameters: pass by ref to const instead of ref to non-const continuation from 40c2700 in dash#4919

…lAndCache

…lock checks contains: - af9ea55 - 2ee7743 (only changes to validation.h)

…o make mempool transaction

excludes: - c9e1a26 (will be added in future fuzzing PR) inapplicable: - 249f43f (we don't have RBF)

inapplicable: - 5cac95c (we don't have RBF)

…hify help text, undashify code comments (#5852) ## Issue being fixed or feature implemented This pull request is a follow-up to [some](#5834 (comment)) [feedback](#5834 (comment)) received on [dash#5834](#5834) as the patterns highlighted were present in different parts of the codebase and hence not corrected within the PR itself but addressed separately. This is that separate PR 🙂 (with some additional cleanup of my own) ## What was done? * This pull request will remain a draft until [dash#5834](#5834) as it will introduce more changes that will need to be corrected in this PR. * Code introduced that is unique to Dash Core (CoinJoin, InstantSend, etc.) has been excluded from un-Dashification as the purpose of it is to reduce backport conflicts, which don't apply in those cases. * `CWallet::CreateTransaction` and the `CreateTransactionTest` fixture have been excluded as the former originates from [dash#3668](#3668) and the latter from [dash#3667](#3667) and are distinct enough to be unique to Dash Core. * There are certain Dashifications and SegWit-removals that prove frustrating as it would break compatibility with programs that rely on the naming of certain keys * `getrawmempool`, `getmempoolancestors`, `getmempooldescendants` and `getmempoolentry` return `vsize` which is currently an alias of `size`. I have been advised to retain `vsize` in lieu of potential future developments. (this was originally remedied in 219a1d0 but has since been dropped) * `getaddressmempool`, `getaddressutxos` and `getaddressdeltas` all return a value with the key `satoshis`. This is frustrating to rename to `duffs` for compatibility reasons. * `decodepsbt` returns (if applicable) `non_witness_utxo` which is frustrating to rename simply to `utxo` for the same reason. * `analyzepsbt` returns (if applicable) `estimated_vsize` which frustrating to rename to `estimated_size` for the same reason. ## How Has This Been Tested? ## Breaking Changes None ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

kwvg force-pushed the atmp_pkg branch 2 times, most recently from b7c5c98 to 7fc608f Compare January 18, 2024 20:26

kwvg mentioned this pull request Jan 18, 2024

backport: merge bitcoin#20332, #20908, #21336, #21443, #21142, #21512, #20833, #21522, #21553, #21970, #24336, partial bitcoin#21606 (fuzzing harness backports: part 7) #5835

Merged

kwvg force-pushed the atmp_pkg branch 2 times, most recently from c7894f6 to 110e975 Compare January 25, 2024 07:21

knst reviewed Jan 26, 2024

View reviewed changes

github-actions bot added the needs rebase label Jan 28, 2024

kwvg force-pushed the atmp_pkg branch from 110e975 to 43029a5 Compare January 29, 2024 15:16

github-actions bot removed the needs rebase label Jan 29, 2024

kwvg force-pushed the atmp_pkg branch from 43029a5 to de915ff Compare January 29, 2024 16:24

kwvg marked this pull request as ready for review January 29, 2024 18:52

kwvg requested review from knst, UdjinM6 and PastaPastaPasta January 29, 2024 18:52

knst reviewed Jan 29, 2024

View reviewed changes

kwvg force-pushed the atmp_pkg branch from de915ff to af74691 Compare January 30, 2024 08:55

UdjinM6 previously approved these changes Jan 30, 2024

View reviewed changes

kwvg requested a review from knst January 31, 2024 08:27

kwvg mentioned this pull request Jan 31, 2024

trivial: add missing rpc help messages, remove segwit references, dashify help text, undashify code comments #5852

Merged

5 tasks

knst reviewed Feb 1, 2024

View reviewed changes

kwvg dismissed UdjinM6’s stale review via db21c86 February 1, 2024 19:19

kwvg force-pushed the atmp_pkg branch from af74691 to db21c86 Compare February 1, 2024 19:19

kwvg requested review from UdjinM6 and knst February 1, 2024 19:19

UdjinM6 approved these changes Feb 1, 2024

View reviewed changes

knst approved these changes Feb 1, 2024

View reviewed changes

UdjinM6 added this to the 20.1 milestone Feb 1, 2024

PastaPastaPasta approved these changes Feb 3, 2024

View reviewed changes

kwvg and others added 12 commits February 2, 2024 23:14

merge bitcoin#19940: Return fee and vsize from testmempoolaccept

9c6c82b

merge bitcoin#19498: Tidy up ProcessOrphanTx

cbbf608

Co-authored-by: Konstantin Akimov <[email protected]>

merge bitcoin#19339: re-delegate absurd fee checking from mempool to …

4acad29

…clients

merge bitcoin#19753: don't add AlreadyHave transactions to recentRejects

2d64620

Co-authored-by: UdjinM6 <[email protected]>

merge bitcoin#20581: Don't make "in" parameters look like "out"/"in-o…

7f535b1

…ut" parameters: pass by ref to const instead of ref to non-const continuation from 40c2700 in dash#4919

merge bitcoin#20834: locks and docs in ATMP and CheckInputsFromMempoo…

5efc930

…lAndCache

merge bitcoin#21062: return MempoolAcceptResult from ATMP

6238ed2

merge bitcoin#21783: Make MempoolAcceptResult members const

0d76475

partial bitcoin#19668: Make runtime lock checks require compile-time …

142790c

…lock checks contains: - af9ea55 - 2ee7743 (only changes to validation.h)

merge bitcoin#21121: Small unit test improvements, including helper t…

e2dda59

…o make mempool transaction

partial bitcoin#20833: enable packages through testmempoolaccept

792b430

excludes: - c9e1a26 (will be added in future fuzzing PR) inapplicable: - 249f43f (we don't have RBF)

merge bitcoin#22084: package testmempoolaccept followups

a7ebe53

inapplicable: - 5cac95c (we don't have RBF)

PastaPastaPasta force-pushed the atmp_pkg branch from db21c86 to a7ebe53 Compare February 3, 2024 05:14

PastaPastaPasta merged commit 544b003 into dashpay:develop Feb 3, 2024
4 of 5 checks passed

thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backport: merge bitcoin#19940, #19498, #19339, #19753, #20581, #20834, #21062, #21783, #21121, #22084, partial bitcoin#19668, #20833 (mempool backports) #5834

backport: merge bitcoin#19940, #19498, #19339, #19753, #20581, #20834, #21062, #21783, #21121, #22084, partial bitcoin#19668, #20833 (mempool backports) #5834

kwvg commented Jan 18, 2024 •

edited

Loading

knst Jan 25, 2024

kwvg Jan 29, 2024

github-actions bot commented Jan 28, 2024

knst Jan 29, 2024

kwvg Jan 30, 2024

UdjinM6 left a comment

knst Feb 1, 2024

kwvg Feb 1, 2024 •

edited

Loading

UdjinM6 left a comment

knst left a comment

PastaPastaPasta left a comment

backport: merge bitcoin#19940, #19498, #19339, #19753, #20581, #20834, #21062, #21783, #21121, #22084, partial bitcoin#19668, #20833 (mempool backports) #5834

backport: merge bitcoin#19940, #19498, #19339, #19753, #20581, #20834, #21062, #21783, #21121, #22084, partial bitcoin#19668, #20833 (mempool backports) #5834

Conversation

kwvg commented Jan 18, 2024 • edited Loading

Additional Information

knst Jan 25, 2024

Choose a reason for hiding this comment

kwvg Jan 29, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 28, 2024

knst Jan 29, 2024

Choose a reason for hiding this comment

kwvg Jan 30, 2024

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

knst Feb 1, 2024

Choose a reason for hiding this comment

kwvg Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

kwvg commented Jan 18, 2024 •

edited

Loading

kwvg Feb 1, 2024 •

edited

Loading