txmempool: Make entry time type-safe (std::chrono) by maflcko · Pull Request #16908 · bitcoin/bitcoin

maflcko · 2019-09-18T17:41:53Z

This changes the type of the entry time of txs into the mempool from int64_t to std::chrono::seconds.

The benefits:

Documents the type for developers
Type violations result in compile errors
After compilation, the two are equivalent (at no run time cost)

cvengler · 2019-09-18T18:41:07Z

Concep ACK,
chrono is definitely better for time managment

DrahtBot · 2019-09-18T20:11:40Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16851 (Continue relaying transactions after they expire from mapRelay by ajtowns)

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.

ajtowns · 2019-09-19T10:43:51Z

ConceptACK

I think LimitMemPoolSize() should have std::chrono::seconds age instead of unsigned long age, which means you can just pass in std::chrono::hours{gArgs.GetArg(...)}; makes it a bit tidier.

Having int64_t as_seconds(std::chrono::seconds t) { return t.count(); } seems like it would be better than using int64_t{xxx.count()} fwiw; that way if we change xxx's type either the conversion happens automatically if there's no loss of precision, or you get a compile time error.

Also, remove the now unused "Mine a single block to get out of IBD". This was fixed in commit 1111aec.

maflcko · 2019-09-19T15:53:41Z

Thx for the review. Took all suggestions by you.

src/rpc/blockchain.cpp

ajtowns · 2019-09-23T12:32:21Z

utACK faec689

laanwj · 2019-09-30T11:40:23Z

src/net_processing.cpp

                connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *mi->second));
                push = true;
-            } else if (pfrom->m_tx_relay->timeLastMempoolReq) {
+            } else if (pfrom->m_tx_relay->m_last_mempool_req.load().count()) {


why not use count_seconds here, if it is preferred to inline .count() (according to the comment)?

The member uses a magic value of 0 to indicate that the mempool was never requested. So this check isn't about time (in a specific type like seconds), but more about checking that any value exists.

Hmm, okay, I guess we'd really want a optional here some day instead of a magic value, but no need to do that here.

Sorry to chime in late -- would it make sense to use the std::chrono::duration::zero value here? I found count() to be unintuitive when I was looking at related code just now (but this std::chrono stuff is new to me so I'm still figuring out how to use it).

Comparing it to zero would also work, but still be odd, as the code is written as if the member were an optional.

I think we can just stop treating it as optional (always assume the last mempool request time for peers was in year 1970). But that can be done in a follow up pull request.

laanwj · 2019-10-02T14:55:21Z

ACK faec689

faec689 txmempool: Make entry time type-safe (std::chrono) (MarcoFalke) faaa1f0 util: Add count_seconds time helper (MarcoFalke) 1111170 test: mempool entry time is persisted (MarcoFalke) Pull request description: This changes the type of the entry time of txs into the mempool from `int64_t` to `std::chrono::seconds`. The benefits: * Documents the type for developers * Type violations result in compile errors * After compilation, the two are equivalent (at no run time cost) ACKs for top commit: ajtowns: utACK faec689 laanwj: ACK faec689 Tree-SHA512: d958e058755d1a1d54cef536a8b30a11cc502b7df0d6ecf84a0ab1d38bc8105a67668a99cd5087a444f6de2421238111c5fca133cdf8e2e2273cb12cb6957845

Summary: Also, remove the now unused "Mine a single block to get out of IBD". This was fixed in commit 1111aecbb5. bitcoin/bitcoin@1111170 --- Depends on D6522 Partial backport of [[bitcoin/bitcoin#16908 | PR16908]] Test Plan: test_runner.py mempool_persist Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6523

Summary: bitcoin/bitcoin@faaa1f0 --- Depends on D6523 Partial backport of Core [[bitcoin/bitcoin#16908 | PR16908]] Test Plan: ninja Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6524

Summary: bitcoin/bitcoin@faec689 --- Depends on D6524 Concludes backport of Core [[bitcoin/bitcoin#16908 | PR16908]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6527

…ono)

merge bitcoin#16117, bitcoin#18358, bitcoin#17383, bitcoin#21052, bitcoin#14424, bitcoin#15159, bitcoin#14689, bitcoin#14978, partial bitcoin#16908, bitcoin#14978, bitcoin#13932: Auxillary Backports

maflcko added Refactoring Mempool labels Sep 18, 2019

maflcko mentioned this pull request Sep 18, 2019

Continue relaying transactions after they expire from mapRelay #16851

Merged

MarcoFalke added 2 commits September 19, 2019 11:30

test: mempool entry time is persisted

1111170

Also, remove the now unused "Mine a single block to get out of IBD". This was fixed in commit 1111aec.

util: Add count_seconds time helper

faaa1f0

maflcko force-pushed the 1909-mempoolEntryChrono branch from fa05d99 to fa18adc Compare September 19, 2019 15:49

ajtowns reviewed Sep 23, 2019

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

txmempool: Make entry time type-safe (std::chrono)

faec689

maflcko force-pushed the 1909-mempoolEntryChrono branch from fa18adc to faec689 Compare September 23, 2019 12:01

laanwj reviewed Sep 30, 2019

View reviewed changes

laanwj merged commit faec689 into bitcoin:master Oct 2, 2019

maflcko deleted the 1909-mempoolEntryChrono branch October 2, 2019 14:56

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

kwvg mentioned this pull request Jul 15, 2021

merge #16117, #18358, #17383, #21052, #14424, #15159, #14689, #14978, partial #16908, #14978, #13932: Auxillary Backports dashpay/dash#4229

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Jul 16, 2021

partial bitcoin#16908: txmempool: Make entry time type-safe (std::chr…

e474e02

…ono)

kwvg added a commit to kwvg/dash that referenced this pull request Aug 5, 2021

partial bitcoin#16908: txmempool: Make entry time type-safe (std::chr…

199a0f8

…ono)

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txmempool: Make entry time type-safe (std::chrono)#16908

txmempool: Make entry time type-safe (std::chrono)#16908
laanwj merged 3 commits intobitcoin:masterfrom
maflcko:1909-mempoolEntryChrono

maflcko commented Sep 18, 2019

Uh oh!

cvengler commented Sep 18, 2019

Uh oh!

DrahtBot commented Sep 18, 2019

Uh oh!

ajtowns commented Sep 19, 2019

Uh oh!

maflcko commented Sep 19, 2019

Uh oh!

Uh oh!

ajtowns commented Sep 23, 2019

Uh oh!

laanwj Sep 30, 2019

Uh oh!

maflcko Sep 30, 2019

Uh oh!

laanwj Oct 2, 2019

Uh oh!

sdaftuar Oct 23, 2019

Uh oh!

maflcko Oct 23, 2019

Uh oh!

laanwj commented Oct 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

maflcko commented Sep 18, 2019

Uh oh!

cvengler commented Sep 18, 2019

Uh oh!

DrahtBot commented Sep 18, 2019

Conflicts

Uh oh!

ajtowns commented Sep 19, 2019

Uh oh!

maflcko commented Sep 19, 2019

Uh oh!

Uh oh!

ajtowns commented Sep 23, 2019

Uh oh!

laanwj Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 2, 2019

Choose a reason for hiding this comment

Uh oh!

sdaftuar Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants