This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here: #28886 (comment).
CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, ajtowns, ismaelsadeeq, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
66+ entry.GetTime().count(),
67+ entry.GetHeight(),
68+ entry.GetSequence(),
69+ entry.GetSpendsCoinbase(),
70+ entry.GetSigOpCost(),
71+ entry.GetLockPoints());
Replacing a simple copy with listing all the details of the object seems cumbersome. If we want to just prevent accidental copies, how about doing something like this instead:
0class CTxMemPoolEntry {
1private:
2 CTxMemPoolEntry(const CTxMemPoolEntry&) = default;
3 struct ExplicitCopyTag { explicit ExplicitCopyTag() = default; };
4public:
5 static constexpr ExplicitCopyTag ExplicitCopy;
6 CTxMemPoolEntry(ExplicitCopyTag /*unused*/, const CTxMemPoolEntry& entry) : CTxMemPoolEntry(entry) { }
7 ...
8};
9
10 mempool_entries.emplace_back(CTxMemPoolEntry::ExplicitCopy, entry);
Thank you @ajtowns,
Updated b8b624b36ebeb167da2f9c6d12f563f6849ccd60 -> 705e3f1de00bf30d728addd52a790a139d948e32 (CTxMemPoolEntryNonCopy_0 -> CTxMemPoolEntryNonCopy_1, compare)
CTxMemPoolEntry only copy-able if a special tag is provided.
This has the goal of prohibiting users from accidentally creating
runtime failures, e.g. by interacting with iterator_to with a copied
entry.
CTxMemPoolEntry is already implicitly not move-constructable. So be
explicit about this and use a std::list to collect the values in the
policy_estimator fuzz test instead of a std::vector.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Would be nice to completely remove the need to create copies, but this involves a lot more code changes. This small fixup makes code and the mentioned code changes easier to review, so further improvements can be done in the future, if there is need and interest.
ACK 705e3f1de00bf30d728addd52a790a139d948e32 🌯
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 705e3f1de00bf30d728addd52a790a139d948e32 🌯
3+9sbfb3Xi5BnQ71dPDaSlA05BlkJvoPm1sFn0t0AAd4IzDC1gW7LUAep5r7LXc1iTT3BUwwHivUxUd3KI62+DA==
For reference, the move constructor is already deleted, according to a compiler:
0./kernel/mempool_entry.h:188:27: note: move constructor of 'CTxMemPoolEntry' is implicitly deleted because field 'm_epoch_marker' has a deleted move constructor
Thanks for the review @maflcko
For reference, the move constructor is already deleted, according to a compiler:
Removed the explicit move constructor deletion again.
Ah sorry, didn’t mean it as a feedback to change the commit. It seems fine to keep it explicitly deleted, because a move is no better than a copy when either results in a runtime error. Having it deleted also prevents it from accidentally appearing again.
Edit: My comment was mostly meant for reviewers that wondered whether and why the move constructor was deleted.
70@@ -71,6 +71,11 @@ class CTxMemPoolEntry
71 typedef std::set<CTxMemPoolEntryRef, CompareIteratorByHash> Children;
72
73 private:
74+ CTxMemPoolEntry(const CTxMemPoolEntry&) = default;
m_parents and m_children need to stay in sync with the container holding this entry – if you leave the copy out of the container, those sets will become bad; if you put it back into the container, those sets will probably be wrong)
