Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts #16400

Merged
merged 1 commit into from Sep 18, 2019

Conversation

@sdaftuar
Copy link
Member

commented Jul 16, 2019

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 sdaftuar referenced this pull request Jul 16, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

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.

Copy link
Member

left a comment

Looked at a109ce2 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.

src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
// Compare a package's feerate against minimum allowed.
bool CheckFeeRate(size_t package_size, CAmount package_fee, CValidationState& state)
{
CAmount mempoolRejectFee = pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(package_size);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 17, 2019

Member

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

Could do the same here.

Suggested change
CAmount mempoolRejectFee = pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(package_size);
CAmount mempoolRejectFee = pool.GetMinFee(m_max_pool_size).GetFee(package_size);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 17, 2019

Author Member

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.

bool& fReplacementTransaction = ws.fReplacementTransaction;
CAmount& nModifiedFees = ws.nModifiedFees;
CAmount& nConflictingFees = ws.nConflictingFees;
size_t& nConflictingSize = ws.nConflictingSize;

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 17, 2019

Member

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.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 17, 2019

Author Member

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?


// Check against previous transactions
// This is done last to help prevent CPU exhaustion denial-of-service attacks.
PrecomputedTransactionData txdata(tx);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 17, 2019

Member

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

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 17, 2019

Author Member

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.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 15, 2019

Contributor

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.

src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
Copy link
Member Author

left a comment

@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?

src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
// Compare a package's feerate against minimum allowed.
bool CheckFeeRate(size_t package_size, CAmount package_fee, CValidationState& state)
{
CAmount mempoolRejectFee = pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(package_size);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 17, 2019

Author Member

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.

bool& fReplacementTransaction = ws.fReplacementTransaction;
CAmount& nModifiedFees = ws.nModifiedFees;
CAmount& nConflictingFees = ws.nConflictingFees;
size_t& nConflictingSize = ws.nConflictingSize;

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 17, 2019

Author Member

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?

src/validation.cpp Outdated Show resolved Hide resolved

// Check against previous transactions
// This is done last to help prevent CPU exhaustion denial-of-service attacks.
PrecomputedTransactionData txdata(tx);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 17, 2019

Author Member

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.

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-07-refactor-atmp branch from ca6c291 to 2bfcbc6 Jul 17, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

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

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-07-refactor-atmp branch from 93e0451 to a372e03 Jul 22, 2019
@DrahtBot DrahtBot removed the Needs rebase label Jul 22, 2019
@MarcoFalke MarcoFalke closed this Jul 23, 2019
@MarcoFalke MarcoFalke reopened this Jul 23, 2019
Copy link
Contributor

left a comment

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

src/validation.cpp Outdated Show resolved Hide resolved
* additions if the associated transaction ends up being rejected by
* the mempool.
*/
std::vector<COutPoint>& m_coins_to_uncache;

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jul 24, 2019

Contributor

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.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 24, 2019

Author Member

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.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jul 25, 2019

Contributor

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.

src/validation.cpp Show resolved Hide resolved
};

// Single transaction acceptance
bool AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args);

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jul 24, 2019

Contributor

Should have EXCLUSIVE_LOCKS_REQUIRED(cs_main)

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 24, 2019

Author Member

Done.

src/validation.cpp Show resolved Hide resolved

// We put the arguments we're handed into a struct, so we can pass them
// around easier.
struct ATMPArgs {

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jul 24, 2019

Contributor

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.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 24, 2019

Author Member

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.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jul 25, 2019

Contributor

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?

Copy link
Member Author

left a comment

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.

src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Show resolved Hide resolved
src/validation.cpp Show resolved Hide resolved
};

// Single transaction acceptance
bool AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args);

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 24, 2019

Author Member

Done.


// We put the arguments we're handed into a struct, so we can pass them
// around easier.
struct ATMPArgs {

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 24, 2019

Author Member

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.

* additions if the associated transaction ends up being rejected by
* the mempool.
*/
std::vector<COutPoint>& m_coins_to_uncache;

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 24, 2019

Author Member

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.

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-07-refactor-atmp branch 2 times, most recently from ec87639 to 0840a1d Jul 29, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

Rebased

@fanquake fanquake removed the Needs rebase label Jul 31, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

ACK 0840a1d (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.

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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.
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg9Qgv/euaSC1DAapYXrYcl9nrFF04qsMeVtv3lJ8hupHNjRXHpTFZoLE2uUO3T
+kf5rugC1UuImZsVOy4odQ0+rVIrnLhrI7DqECzQ+P7OX4+JN/Wf6dfciHzznGsX
h+lrDEDgs0MuCEgsoQAIgQe39IDsexNQyE5i6+dEZtbqhhiuT+hoJAH8Hj3TKQib
60Sn7yBpdOSgYnZM1TTELCYSL4lDzXIFH5lW3ojj2oYF+1AnAqjRU8EydUH0wGRX
sK1PtCdI8dXhTXChHYnku24e0cR1VCHMjPmLXXxtqp9xm0qVtDMH7coSgOdY9SRx
OO++RjA+8oag5YmCxZRVcvkZ7m/XlwRshGJDbKCSKy2vuW++MNJmTzkxUkZPUl7s
BSrBP04DYMyhEtZHfcMV3oLbb8HZ/a9RJTbBlpQubEsvBT5SbcPdFMhMv7Ok6M6C
SSQm56IF2bz/Ky6F1F1h1EVnVNpF1SXkXNHfnn5JfZoPVlbvvNk5k+iS7pVqxtOI
5TNLNS1L
=ZwOV
-----END PGP SIGNATURE-----

Timestamp of file with hash e1313ca99cc4b41a7b44fb90863a0d30e70235c91504a644337a0c8a77443411 -

// limiting is performed, false otherwise.
bool Finalize(ATMPArgs& args, Workspace& ws) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);

// Compare a package's feerate against minimum allowed.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 7, 2019

Member

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

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Aug 7, 2019

Member

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

utACK 0840a1d.

@MarcoFalke MarcoFalke added this to the 0.19.0 milestone Aug 14, 2019
Copy link
Contributor

left a comment

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


// Check against previous transactions
// This is done last to help prevent CPU exhaustion denial-of-service attacks.
PrecomputedTransactionData txdata(tx);

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 15, 2019

Contributor

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.

std::set<uint256>& setConflicts = ws.m_conflicts;
CTxMemPool::setEntries& allConflicting = ws.m_all_conflicting;
CTxMemPool::setEntries& setAncestors = ws.m_ancestors;
std::unique_ptr<CTxMemPoolEntry>& entry = ws.m_entry;

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 15, 2019

Contributor

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)

@fanquake fanquake added this to Chasing Concept ACK in High-priority for review Aug 15, 2019
@fanquake fanquake moved this from Chasing Concept ACK to Blockers in High-priority for review Aug 15, 2019
@sdaftuar sdaftuar force-pushed the sdaftuar:2019-07-refactor-atmp branch from 0840a1d to 6e43129 Sep 3, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

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 sdaftuar reopened this Sep 3, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Could squash the commits?

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgkugv9F0lQN2nYMHaZYwMzt6PRXjCwd/IslX9phZJlT9vZewf/nRk8kArEaaaX
xYuku345NFSk5wjmMLx0ZkGKAdn2GFvi+oONYh953nL+Jqf9C9sLT3yLU1AwEnlR
pWL+UpjSRGDJGQNvdhe/1Q7c2/ZtLEkZSQkm3HHskMj9MF2pik/sOX9gGwpDGTL8
+QOLeFlmC9O8nUTjbs6AYQexjvLsS7gAVAfYW8DWeJsmHbVe/GY5bjRpJj5FTBWD
od7gWTs7TVg88tstH8w2EI0L04aHDsoAEc22UpxFRA/w7sGHuNxmrBUBjrxoEI3P
4GYRr7uQXVXg9rpTrMYIkMkwv3xOOtVslMZDLKzg9QqdKWrkbK29hogHHnaquPJd
JrBvAkLQL2CNorNH3YOJDnIVb0gawZTS9L/T3TrtOeeGd+BlQj/1waY2HxNP1DVi
1IMH2A2RAPrpzz3CTKQI9bwtCAV42uJ5bDiAUppcoU0GlymqvvguYjrfC/up8Iar
urmEkETC
=GMTX
-----END PGP SIGNATURE-----

Timestamp of file with hash 4006c7685a375c330b238c9bed57e7537aef882d3af5b0be44fcdd0b893c1c57 -

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-07-refactor-atmp branch from 4c17623 to ef79e05 Sep 3, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

Squashed

@DrahtBot DrahtBot removed the Needs rebase label Sep 3, 2019
@fanquake fanquake changed the title [refactor] Rewrite AcceptToMemoryPoolWorker() using smaller parts refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts Sep 6, 2019
@laanwj laanwj modified the milestones: 0.19.0, 0.20.0 Sep 11, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

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.

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 sdaftuar force-pushed the sdaftuar:2019-07-refactor-atmp branch from ef79e05 to 4a87c5c Sep 16, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

Rebased (old version is 16400.2)

@DrahtBot DrahtBot removed the Needs rebase label Sep 16, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjSRAv9Gg+QHZvblyy4FHN6sRMj+h49EyMuE4ZZfQIbdISoiIQggyDYUQpi1S8K
gtLYc71DYaMO7eNec+pmrBvEiwM3VUFMUePuO3ZE6cNkL8ch3VKdkm8/M9vLriDk
cJjqYKEe+dCUj/mLIzCRZUxs13P0DaQjowiiqbhckwgfejXSILu6Gb34Ykto07Eo
2N9LUOCT5yPX+oxCOLKi2ekKBcUPl1FfZpWTf+8BwQGzXCE7SwJYBbeDVBWdW9TG
5iSrEyoPaLyXJdkDV4SMqwDoaGeK2iTAczh8smM+fpDh6v5m0OYYEtOg/GvN+11z
TJkkrVecwoThJI/qjuWw3PPQGJR/7Rehpo7gf+RBpbp/0KzYspw7JKhS+U30EBjq
GloMYN94xQzkxxs3K166bl0SPo+AePDdFM+JBmisTwCiBUbFXF6RIZklA+vqZpM6
OsG4WgOUHjPO3q0PrNtdd0fl6HtCu19dbasfURcJ6fYprNmwPdqgpc11Mq202ajg
mJKxJZDS
=8KLl
-----END PGP SIGNATURE-----

Timestamp of file with hash 1e4042da15dcf1e1e7c77742a411deedbfd643673230755ac10c03edcad1c0d0 -

@jonatack jonatack referenced this pull request Sep 17, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

ACK 4a87c5c

laanwj added a commit that referenced this pull request Sep 18, 2019
…ler parts

4a87c5c [refactor] Rewrite AcceptToMemoryPoolWorker() using smaller parts (Suhas Daftuar)

Pull request description:

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

ACKs for top commit:
  MarcoFalke:
    re-ACK 4a87c5c (did the rebase myself and arrived at the same result, mod whitespace)
  laanwj:
    ACK 4a87c5c

Tree-SHA512: b0495c026ffe06146258bace3d5e0c9aaf23fa65f89f258abc4af5980812e68e63a799f1d923e78ac1ee6bcafaf1222b2c2690a527df9b65dff7b48a013f154e
@laanwj laanwj merged commit 4a87c5c into bitcoin:master Sep 18, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcoFalke MarcoFalke removed this from the 0.20.0 milestone Sep 18, 2019
domob1812 added a commit to domob1812/namecore that referenced this pull request Sep 23, 2019
Merge the upstream refactoring to AcceptToMemoryPoolWorker from
bitcoin/bitcoin#16400 with the Namecoin-specific
changes made to it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.