refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts

sdaftuar commented at 8:15 pm on July 16, 2019: member

This is in preparation for re-using these validation components for a new version of AcceptToMemoryPool() that can operate on multiple transactions (“package relay”).

sdaftuar commented at 8:18 pm on July 16, 2019: member

The motivation behind this refactor is to allow implementation of a package relay system (#14895). A simple implementation that I have in mind, which requires no additional p2p protocol changes, is shown at #16401.

DrahtBot commented at 10:33 pm on July 16, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16658 (validation: Rename CheckInputs to CheckInputScripts by jnewbery)
#13525 (policy: Report reason inputs are nonstandard from AreInputsStandard by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake added the label Validation on Jul 16, 2019

fanquake added the label Refactoring on Jul 16, 2019

in src/validation.cpp:524 in a109ce24d7 outdated

529+
530+    // The package limits in effect at the time of invocation.
531+    size_t nLimitAncestors{0};
532+    size_t nLimitAncestorSize{0};
533+    size_t nLimitDescendants{0};
534+    size_t nLimitDescendantSize{0};

MarcoFalke commented at 11:50 am on July 17, 2019:

Not sure why this is set to 0 as default, as opposed to the actual DEFAULT_* values. I understand that setting the defaults here is very verbose and not needed, so why not make it const, so that the compiler enforces that this is set in the constructor?

0    const size_t nLimitDescendantSize;

sdaftuar commented at 2:57 pm on July 17, 2019:

Sounds good, thanks!

in src/validation.cpp:440 in a109ce24d7 outdated

445+     *                                coins cache, but were added as a result of validating the tx
446+     *                                for mempool acceptance. This allows the caller to optionally
447+     *                                remove the cache additions if the associated transaction ends
448+     *                                up being rejected by the mempool.
449+     */
450+    bool AcceptSingleTransaction(const CChainParams& chainparams, CTxMemPool& pool,

MarcoFalke commented at 11:53 am on July 17, 2019:

Can you explain the difference between pool and this->pool? Looks like passing in the pool as a function parameter is redundant and confusing. Might want to drop it?

Also, I’d slightly prefer to call the member m_tx_pool or something. We already use mempool for the global and pool for things passed as parameters.

0    bool AcceptSingleTransaction(const CChainParams& chainparams,

sdaftuar commented at 2:57 pm on July 17, 2019:

Passing the mempool into AcceptSingleTransaction was just a mistake, removing.

I’ll add a commit that fixes all the variable naming to conform to the style guide.

in src/validation.cpp:445 in a109ce24d7 outdated

450+    bool AcceptSingleTransaction(const CChainParams& chainparams, CTxMemPool& pool,
451+            CValidationState& state, const CTransactionRef& ptx,
452+            bool* pfMissingInputs, int64_t nAcceptTime,
453+            std::list<CTransactionRef>* plTxnReplaced, bool bypass_limits,
454+            const CAmount& nAbsurdFee,
455+            std::vector<COutPoint>& coins_to_uncache, bool test_accept);

MarcoFalke commented at 11:56 am on July 17, 2019:

Looks like all the remaining args can be passed as a ATMPArgs. Any reason not to do this?

0            ATMPArgs& args);

sdaftuar commented at 3:07 pm on July 17, 2019:

Agreed - looks better that way.

in src/validation.cpp:459 in a109ce24d7 outdated

464+        int64_t nAcceptTime;
465+        std::list<CTransactionRef>* plTxnReplaced;
466+        bool bypass_limits;
467+        const CAmount& nAbsurdFee;
468+        std::vector<COutPoint>& coins_to_uncache;
469+        bool test_accept;

MarcoFalke commented at 11:57 am on July 17, 2019:

Could better document which arguments are going in and which are going out by setting everything that goes in to const?

0        const bool test_accept;

sdaftuar commented at 3:07 pm on July 17, 2019:

Sounds good.

in src/validation.cpp:503 in a109ce24d7 outdated

508+    bool Finalize(ATMPArgs& args, const CTransactionRef& ptx, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs);
509+
510+    // Compare a package's feerate against minimum allowed.
511+    bool CheckFeeRate(size_t package_size, CAmount package_fee, CValidationState& state)
512+    {
513+        CAmount mempoolRejectFee = pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(package_size);

MarcoFalke commented at 12:00 pm on July 17, 2019:

Why are some of the settings parsed in the constructor and saved as a member and others are not?

Could do the same here.

0        CAmount mempoolRejectFee = pool.GetMinFee(m_max_pool_size).GetFee(package_size);

sdaftuar commented at 3:21 pm on July 17, 2019:

The reason is that I end up needing to access the package size limits directly when adding AcceptMultipleTransactions() in #16401, whereas I didn’t need local copies of the reject fee. So my main motivation behind not saving these as members is to avoid cluttering the state, which was already complex to untangle.

I don’t feel strongly about this though; I just wanted to make it as easy as possible for reviewers and future code maintainers to be able to understand what data is required by each function.

in src/validation.cpp:548 in a109ce24d7 outdated

556+    CTxMemPool::setEntries& setAncestors = ws.setAncestors;
557+    std::unique_ptr<CTxMemPoolEntry>& entry = ws.entry;
558+    bool& fReplacementTransaction = ws.fReplacementTransaction;
559+    CAmount& nModifiedFees = ws.nModifiedFees;
560+    CAmount& nConflictingFees = ws.nConflictingFees;
561+    size_t& nConflictingSize = ws.nConflictingSize;

MarcoFalke commented at 12:09 pm on July 17, 2019:

Given that the local name and the name of the member are identical, does it really make sense to alias it? Looks like you already changed every line in this function due to whitespace fixups, so might as well prefix what we need with ws. or args. in line. No strong opinion, though. If you decide to keep the current approach, I think you can make review easier by not duplicating all the types (use auto&, which should keep the type and constness as is.

sdaftuar commented at 3:25 pm on July 17, 2019:

I did think it was going to be easier for review if I didn’t end up changing all the variables out at the same time. I think my preferred approach would be to fix the variable names in MemPoolAccept, Workspace, and ATMPArgs to conform to the style guide, but then continue to use these aliases so that we’re not also changing all the variables in each of these functions.

One benefit of the aliases is that it makes it easier to tell what variables we’re not using as well. I could also achieve that by changing the functions so that we pass in less data, such as by directly passing in only those members of the Workspace and Args structs that each function needs. Perhaps that is a better approach?

in src/validation.cpp:821 in a109ce24d7 outdated

1002 
1003-        constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS;
1004-
1005-        // Check against previous transactions
1006-        // This is done last to help prevent CPU exhaustion denial-of-service attacks.
1007-        PrecomputedTransactionData txdata(tx);

MarcoFalke commented at 12:20 pm on July 17, 2019:

Any reason this is not stored in the workspace at this point? Maybe as a unique ptr or something, like the entry?

sdaftuar commented at 3:27 pm on July 17, 2019:

I had a version of this code where it was a unique_ptr, but thought this was an easy way to avoid a heap allocation that we don’t really need.

ajtowns commented at 7:18 am on August 15, 2019:

Seems a bit weird to have m_entry as a unique_ptr, but not have txdata in there as well just to avoid it being on the heap. I think making txdata a unique_ptr in Workspace would simplify #16401 a bit too.

in src/validation.cpp:474 in a109ce24d7 outdated

479+        std::unique_ptr<CTxMemPoolEntry> entry;
480+
481+        bool fReplacementTransaction{false};
482+        CAmount nModifiedFees{0};
483+        CAmount nConflictingFees{0};
484+        size_t nConflictingSize{0};

MarcoFalke commented at 12:22 pm on July 17, 2019:

I’d prefer not to initialize the memory, so that valgrind and other memory tools can warn about logic errors in the code.

sdaftuar commented at 3:18 pm on July 17, 2019:

I’m not convinced that removing default initialization is a win, but I’m also indifferent, so I’ll take your preference.

in src/validation.cpp:644 in a109ce24d7 outdated

678+    // Bring the best block into scope
679+    view.GetBestBlock();
680 
681-        // we have all inputs cached now, so switch back to dummy, so we don't need to keep lock on mempool
682-        view.SetBackend(dummy);
683+    // XXX: this comment is wrong/confusing, since we do retain a lock on the mempool

MarcoFalke commented at 12:25 pm on July 17, 2019:

Not sure what you are trying to say with the XXX:. I feel like the documentation “typo” can be fixed in a separate commit or pull request.

sdaftuar commented at 3:26 pm on July 17, 2019:

Yeah I just wanted to call attention to it being wrong; given that we do keep a lock on the mempool throughout testing for acceptance, I wonder if we should just scrap the whole idea of using the dummy at all?

sdaftuar commented at 1:32 pm on July 18, 2019:

Cleaned this up by changing the comment to give a reason why we might actually want to still use a dummy here, thanks @MarcoFalke

MarcoFalke approved

MarcoFalke commented at 12:29 pm on July 17, 2019: member

Looked at a109ce24d719d709b4f2d3d129a165a9323a32b5 on GitHub. I think it was attempted in the past to split up this 400 line function into smaller parts, but now there is an actual motivation for it (#16401). I left some style questions inline. Feel free to ignore.

in src/validation.cpp:447 in 52e932e68c outdated

452+    };
453+
454+    /**
455+     * Single transaction acceptance
456+     *
457+     * @param[out] coins_to_uncache   Return any outpoints which were not previously present in the

MarcoFalke commented at 5:51 pm on July 17, 2019:

Could move this doc to ATMPArgs::coins_to_uncache?

sdaftuar commented at 7:03 pm on July 17, 2019: member

@MarcoFalke Thanks for the quick review! I’ve pushed many updates to address feedback.

I plan to squash many of the cleanup commits down, but I wonder if the variable rename is easier to review by itself?

sdaftuar force-pushed on Jul 17, 2019

sdaftuar commented at 7:08 pm on July 17, 2019: member

I went ahead and squashed the commits up to the variable rename, unsquashed version is here: 16400.1

DrahtBot added the label Needs rebase on Jul 19, 2019

sdaftuar force-pushed on Jul 22, 2019

DrahtBot removed the label Needs rebase on Jul 22, 2019

MarcoFalke closed this on Jul 23, 2019

MarcoFalke reopened this on Jul 23, 2019

in src/validation.cpp:7 in a372e03e89 outdated

3@@ -4,6 +4,7 @@
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 
6 #include <validation.h>
7+#include <memory>

ajtowns commented at 5:59 am on July 24, 2019:

Doesn’t seem necessary?

sdaftuar commented at 1:56 pm on July 24, 2019:

I have no idea now why I thought I needed it, gone.

in src/validation.cpp:459 in a372e03e89 outdated

457+         * cache, but were added as a result of validating the tx for mempool
458+         * acceptance. This allows the caller to optionally remove the cache
459+         * additions if the associated transaction ends up being rejected by
460+         * the mempool.
461+         */
462+        std::vector<COutPoint>& m_coins_to_uncache;

ajtowns commented at 6:03 am on July 24, 2019:

Seems like m_coins_to_uncache could just be a member var of MemPoolAccept directly; with either a UncacheCoins() method invoked by the caller on failure, or having a ~MemPoolAccept() destructor that automatically uncaches, with AcceptSingleTransaction calling m_coins_to_uncache.clear() before returning true to avoid uncaching things that should remain cached.

sdaftuar commented at 2:33 pm on July 24, 2019:

I’m not sure it makes sense to mix ATMP logic at this layer with managing the utxo cache – I think that’s better handled by callers in the long run, even we’re just doing something very simple now.

ajtowns commented at 0:41 am on July 25, 2019:

Hmm, I was thinking of it more as “ATMP logic messes up the cache when it decides it can’t accept, so cleaning up that mess should also be part of ATMP logic”. But having it be done in ATMPWithTime is still part of ATMP in my book, so either way works.

in src/validation.cpp:469 in a372e03e89 outdated

467+    bool AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args);
468+
469+private:
470+    // All the intermediate state that gets passed between the various levels
471+    // of checking a given transaction.
472+    struct Workspace {

ajtowns commented at 6:07 am on July 24, 2019:

Could emphasise that this is per-transaction workspace, as opposed to m_view, m_viewmempool and m_dummy

ajtowns commented at 6:13 am on July 24, 2019:

Could set const CTransaction& m_tx = *ptx in Workspace, to save passing ptx around as an argument. Once you get to AcceptMultipleTransactions, I think this would let you just iterate over tx_workspaces instead of having to simultaneously iterate over tx_list and tx_workspaces which would make that code a bit simpler.

sdaftuar commented at 2:15 pm on July 24, 2019:

There’s one place in PreChecks() where we need ptx, but we can cache that in Workspace at least. Done.

in src/validation.cpp:457 in a372e03e89 outdated

462+        std::vector<COutPoint>& m_coins_to_uncache;
463+        const bool m_test_accept;
464+    };
465+
466+    // Single transaction acceptance
467+    bool AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args);

ajtowns commented at 6:08 am on July 24, 2019:

Should have EXCLUSIVE_LOCKS_REQUIRED(cs_main)

sdaftuar commented at 2:15 pm on July 24, 2019:

Done.

in src/validation.cpp:433 in a372e03e89 outdated

431- *                                up being rejected by the mempool.
432- */
433-static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool& pool, CValidationState& state, const CTransactionRef& ptx,
434-                              bool* pfMissingInputs, int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
435-                              bool bypass_limits, const CAmount& nAbsurdFee, std::vector<COutPoint>& coins_to_uncache, bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
436+class MemPoolAccept

ajtowns commented at 6:09 am on July 24, 2019:

Probably should have namespace { class MemPoolAccept { }; }; so these methods get internal linkage, to match the “static” they used to have.

sdaftuar commented at 1:56 pm on July 24, 2019:

Sounds good.

in src/validation.cpp:444 in a372e03e89 outdated

442+        m_limit_descendants(gArgs.GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT)),
443+        m_limit_descendant_size(gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000) {}
444+
445+    // We put the arguments we're handed into a struct, so we can pass them
446+    // around easier.
447+    struct ATMPArgs {

ajtowns commented at 6:15 am on July 24, 2019:

I think you could make it even easier by having const ATMPArgs m_args as a member of MemPoolAccept, and not passing it around at all. That doesn’t work for m_bypass_limits in AcceptMultipleTransactions, but you’re basically manually checking the limits there anyway, so maybe could do false for all of them; otherwise could use Workspace for per-tx logic.

sdaftuar commented at 2:31 pm on July 24, 2019:

I think I’m going to leave this as-is for the moment. I do think that holistically, we can think about doing one of two things with this refactor:

(a) go all in for storing state in the class and not pass around data between member functions (b) dump this class and pass state around explicitly instead

I did something weird in this PR where I started to introduce a class (mostly to function as a namespace, I guess!), but I mostly clung to the historical style of passing everything around anyway (except for the mempool/coinsview related things, and the chain limits). So this is probably a pretty weird state to leave things, and I’m happy to dive down either direction so that the design is more cohesive, but I’d like feedback on which way to go.

ajtowns commented at 1:12 am on July 25, 2019:

I guess I think either way is fine, but my preference is to keep the class for namespacing (so it’s clear the private methods are only called from the public ones), and to keep the number of explicit params to the functions pretty small – pulling the stuff you actually need from the Args/Workspace structs like you do looks good to me.

Maybe it might make sense to only pass things as args if they aren’t known got MemPoolAccept’s constructor or can be deallocated before the end – like txdata – or are per-transaction – like Workspace?

ajtowns commented at 6:46 am on July 24, 2019: member

Concept ACK. Looks pretty good; ATMPArgs seems helpful. I’ve only looked at the structure, not checked the logic.

sdaftuar commented at 6:30 pm on July 24, 2019: member

Thanks @ajtowns for the review. I’ve addressed most comments; the main conceptual point I would like feedback on is whether wrapping the state in this MemPoolAccept() class is a good idea, or if I should go back to the style we have historically used of passing state explicitly to these functions (as I mention below).

Also, I’ve updated the related pull over at #16401 to be rebased on the current version of this PR, so we can track how these changes affect the goal. Seems to work fine so far.

DrahtBot added the label Needs rebase on Jul 29, 2019

sdaftuar force-pushed on Jul 29, 2019

sdaftuar force-pushed on Jul 31, 2019

sdaftuar commented at 2:20 pm on July 31, 2019: member

Rebased

fanquake removed the label Needs rebase on Jul 31, 2019

in src/validation.cpp:506 in 0840a1d2e2 outdated

510+    // Try to add the transaction to the mempool, removing any conflicts first.
511+    // Returns true if the transaction is in the mempool after any size
512+    // limiting is performed, false otherwise.
513+    bool Finalize(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);
514+
515+    // Compare a package's feerate against minimum allowed.

MarcoFalke commented at 5:24 pm on August 7, 2019:

note to other reviewers. This says “package”, but in the current code this is a single tx (the one that is added)

in src/validation.cpp:488 in 0840a1d2e2 outdated

492+    };
493+
494+    // Run the policy checks on a given transaction, excluding any script checks.
495+    // Looks up inputs, calculates feerate, considers replacement, evaluates
496+    // package limits, etc. As this function can be invoked for "free" by a peer,
497+    // only tests that are fast should be done here (to avoid CPU DoS).

MarcoFalke commented at 5:33 pm on August 7, 2019:

doc-nit: Should this say that this method fully initializes the ws? Or is this only a coincidence not worth to mention?

MarcoFalke commented at 6:03 pm on August 7, 2019: member

ACK 0840a1d2e2270c06342710c4ebfaac1c41652dab (read diff with –ignore-all-space)

I like that this splits up the massivley large ATMP into logical chunks (not only functional separate checks, but also structural into args and workspace). With the added documentation, the code is probably easier to understand now.

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 0840a1d2e2270c06342710c4ebfaac1c41652dab (read diff with --ignore-all-space)
 4
 5I like that this splits up the massivley large ATMP into logical chunks
 6(not only functional separate checks, but also structural into args and
 7workspace). With the added documentation, the code is probably easier to
 8understand now.
 9-----BEGIN PGP SIGNATURE-----
10
11iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
12pUg9Qgv/euaSC1DAapYXrYcl9nrFF04qsMeVtv3lJ8hupHNjRXHpTFZoLE2uUO3T
13+kf5rugC1UuImZsVOy4odQ0+rVIrnLhrI7DqECzQ+P7OX4+JN/Wf6dfciHzznGsX
14h+lrDEDgs0MuCEgsoQAIgQe39IDsexNQyE5i6+dEZtbqhhiuT+hoJAH8Hj3TKQib
1560Sn7yBpdOSgYnZM1TTELCYSL4lDzXIFH5lW3ojj2oYF+1AnAqjRU8EydUH0wGRX
16sK1PtCdI8dXhTXChHYnku24e0cR1VCHMjPmLXXxtqp9xm0qVtDMH7coSgOdY9SRx
17OO++RjA+8oag5YmCxZRVcvkZ7m/XlwRshGJDbKCSKy2vuW++MNJmTzkxUkZPUl7s
18BSrBP04DYMyhEtZHfcMV3oLbb8HZ/a9RJTbBlpQubEsvBT5SbcPdFMhMv7Ok6M6C
19SSQm56IF2bz/Ky6F1F1h1EVnVNpF1SXkXNHfnn5JfZoPVlbvvNk5k+iS7pVqxtOI
205TNLNS1L
21=ZwOV
22-----END PGP SIGNATURE-----

Timestamp of file with hash e1313ca99cc4b41a7b44fb90863a0d30e70235c91504a644337a0c8a77443411 -

TheBlueMatt commented at 8:11 pm on August 14, 2019: member

utACK 0840a1d2e2270c06342710c4ebfaac1c41652dab.

MarcoFalke added this to the milestone 0.19.0 on Aug 14, 2019

in src/validation.cpp:553 in 0840a1d2e2 outdated

555+
556+    // Alias what we need out of ws
557+    std::set<uint256>& setConflicts = ws.m_conflicts;
558+    CTxMemPool::setEntries& allConflicting = ws.m_all_conflicting;
559+    CTxMemPool::setEntries& setAncestors = ws.m_ancestors;
560+    std::unique_ptr<CTxMemPoolEntry>& entry = ws.m_entry;

ajtowns commented at 8:03 am on August 15, 2019:

Would make for a slightly smaller diff to say CTxMemPoolEntry& entry = *ws.m_entry; and only declare it after m_entry has been reset (and assert(wx.m_entry); beforehand in Finalize to keep the static analysis tools happy, I suppose)

ajtowns approved

ajtowns commented at 8:07 am on August 15, 2019: member

ACK 0840a1d2e2270c06342710c4ebfaac1c41652dab ; added some suggestions, but this is already a good improvement. re-reviewed the code, checked it compiled and tests passed.

fanquake added this to the "Chasing Concept ACK" column in a project

fanquake moved this from the "Chasing Concept ACK" to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Aug 15, 2019

laanwj removed this from the "Blockers" column in a project

sdaftuar closed this on Sep 3, 2019

sdaftuar force-pushed on Sep 3, 2019

sdaftuar commented at 7:36 pm on September 3, 2019: member

Oops. (For future reference, if you accidentally push master to your branch because, say, you didn’t actually finish the rebase before you pushed, then the PR will autoclose.)

sdaftuar reopened this on Sep 3, 2019

in src/validation.cpp:820 in 5bacbfa0c9 outdated

1046-        constexpr unsigned int scriptVerifyFlags = STANDARD_SCRIPT_VERIFY_FLAGS;
1047-
1048-        // Check against previous transactions
1049-        // The first loop above does all the inexpensive checks.
1050-        // Only if ALL inputs pass do we perform expensive ECDSA signature checks.
1051-        // Helps prevent CPU exhaustion denial-of-service attacks.

MarcoFalke commented at 7:54 pm on September 3, 2019:

Any reason you remove this comment here?

sdaftuar commented at 7:57 pm on September 3, 2019:

Not in particular, I think with the refactor the structure made it more clear that we do the CPU intensive operations after the non-CPU intensive ones, but I can resurrect this comment in AcceptSingleTransaction.

MarcoFalke commented at 8:07 pm on September 3, 2019: member

Could squash the commits?

re-ACK 4c176230dbe1d6c7f507f9983dc40c48b39766ba (did the rebase myself and came to the same result)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 4c176230dbe1d6c7f507f9983dc40c48b39766ba (did the rebase myself and came to the same result)
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgkugv9F0lQN2nYMHaZYwMzt6PRXjCwd/IslX9phZJlT9vZewf/nRk8kArEaaaX
 8xYuku345NFSk5wjmMLx0ZkGKAdn2GFvi+oONYh953nL+Jqf9C9sLT3yLU1AwEnlR
 9pWL+UpjSRGDJGQNvdhe/1Q7c2/ZtLEkZSQkm3HHskMj9MF2pik/sOX9gGwpDGTL8
10+QOLeFlmC9O8nUTjbs6AYQexjvLsS7gAVAfYW8DWeJsmHbVe/GY5bjRpJj5FTBWD
11od7gWTs7TVg88tstH8w2EI0L04aHDsoAEc22UpxFRA/w7sGHuNxmrBUBjrxoEI3P
124GYRr7uQXVXg9rpTrMYIkMkwv3xOOtVslMZDLKzg9QqdKWrkbK29hogHHnaquPJd
13JrBvAkLQL2CNorNH3YOJDnIVb0gawZTS9L/T3TrtOeeGd+BlQj/1waY2HxNP1DVi
141IMH2A2RAPrpzz3CTKQI9bwtCAV42uJ5bDiAUppcoU0GlymqvvguYjrfC/up8Iar
15urmEkETC
16=GMTX
17-----END PGP SIGNATURE-----

Timestamp of file with hash 4006c7685a375c330b238c9bed57e7537aef882d3af5b0be44fcdd0b893c1c57 -

sdaftuar force-pushed on Sep 3, 2019

sdaftuar commented at 8:10 pm on September 3, 2019: member

Squashed

DrahtBot removed the label Needs rebase on Sep 3, 2019

fanquake renamed this:
~~[refactor] Rewrite AcceptToMemoryPoolWorker() using smaller parts~~
refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts
on Sep 6, 2019

DrahtBot added the label Needs rebase on Sep 7, 2019

laanwj removed this from the milestone 0.19.0 on Sep 11, 2019

laanwj added this to the milestone 0.20.0 on Sep 11, 2019

laanwj removed this from the milestone 0.20.0 on Sep 11, 2019

laanwj added this to the milestone 0.19.0 on Sep 11, 2019

laanwj removed this from the milestone 0.19.0 on Sep 11, 2019

laanwj added this to the milestone 0.20.0 on Sep 11, 2019

laanwj commented at 7:01 am on September 11, 2019: member

The motivation behind this refactor is to allow implementation of a package relay system (#14895)

Bumped the milestone to 0.20: as noted by @fanquake, as this is preparation work for a feature, it doesn’t make a lot of sense to merge this last-minute for 0.19, so I think we should merge this early in the 0.20 release cycle. (on the other hand this has two ACKs so if people feel this is ready for merge already feel free to disagree)

MarcoFalke commented at 8:32 am on September 11, 2019: member

I think it makes sense to merge this for 0.19 to make policy backports to 0.19 easier @sdaftuar Are you still working on this?

sdaftuar commented at 8:38 am on September 11, 2019: member

I independently figured we’d wait until 0.20 for the same reason @laanwj gave, but I can get this rebased sooner for consideration in 0.19.

[refactor] Rewrite AcceptToMemoryPoolWorker() using smaller parts

This is in preparation for re-using these validation components for a new
version of AcceptToMemoryPool() that can operate on multiple transactions
("package relay").

4a87c5cfdf

sdaftuar force-pushed on Sep 16, 2019

sdaftuar commented at 3:39 pm on September 16, 2019: member

Rebased (old version is 16400.2)

DrahtBot removed the label Needs rebase on Sep 16, 2019

MarcoFalke commented at 6:17 pm on September 16, 2019: member

re-ACK 4a87c5cfdf7dd72d999ebeaf17db6695a7c6298d (did the rebase myself and arrived at the same result, mod whitespace)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 4a87c5cfdf7dd72d999ebeaf17db6695a7c6298d (did the rebase myself and arrived at the same result, mod whitespace)
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjSRAv9Gg+QHZvblyy4FHN6sRMj+h49EyMuE4ZZfQIbdISoiIQggyDYUQpi1S8K
 8gtLYc71DYaMO7eNec+pmrBvEiwM3VUFMUePuO3ZE6cNkL8ch3VKdkm8/M9vLriDk
 9cJjqYKEe+dCUj/mLIzCRZUxs13P0DaQjowiiqbhckwgfejXSILu6Gb34Ykto07Eo
102N9LUOCT5yPX+oxCOLKi2ekKBcUPl1FfZpWTf+8BwQGzXCE7SwJYBbeDVBWdW9TG
115iSrEyoPaLyXJdkDV4SMqwDoaGeK2iTAczh8smM+fpDh6v5m0OYYEtOg/GvN+11z
12TJkkrVecwoThJI/qjuWw3PPQGJR/7Rehpo7gf+RBpbp/0KzYspw7JKhS+U30EBjq
13GloMYN94xQzkxxs3K166bl0SPo+AePDdFM+JBmisTwCiBUbFXF6RIZklA+vqZpM6
14OsG4WgOUHjPO3q0PrNtdd0fl6HtCu19dbasfURcJ6fYprNmwPdqgpc11Mq202ajg
15mJKxJZDS
16=8KLl
17-----END PGP SIGNATURE-----

Timestamp of file with hash 1e4042da15dcf1e1e7c77742a411deedbfd643673230755ac10c03edcad1c0d0 -

laanwj commented at 2:15 pm on September 18, 2019: member

ACK 4a87c5cfdf7dd72d999ebeaf17db6695a7c6298d

laanwj referenced this in commit 408c920381 on Sep 18, 2019

laanwj merged this on Sep 18, 2019

laanwj closed this on Sep 18, 2019

MarcoFalke removed this from the milestone 0.20.0 on Sep 18, 2019

domob1812 referenced this in commit eaa69e89c9 on Sep 23, 2019

JeremyRubin added this to the "Package Relay" column in a project

Fabcien referenced this in commit 3b83c27d9a on Dec 8, 2020

glozow referenced this in commit 317a9c63c4 on Mar 23, 2021

glozow referenced this in commit 268965da59 on Apr 1, 2021

glozow referenced this in commit 4eca20d6f7 on Apr 1, 2021

DrahtBot locked this on Dec 16, 2021

refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts #16400

Conflicts