validation: use std::chrono throughout some validation functions #19088

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:validation_chrono changing 1 files +14 −14

fanquake commented at 7:50 AM on May 28, 2020: member

Probably up for debate as to which type is used for the constants. Personally, swapping these to hours is more readable.
fanquake added the label Validation on May 28, 2020
practicalswift commented at 10:15 AM on May 28, 2020: contributor

Concept ACK: suggestion version is more readable
in src/validation.cpp:350 in b661af29df outdated
```
 346 | @@ -347,7 +347,7 @@ static bool IsCurrentForFeeEstimation() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 347 |      AssertLockHeld(cs_main);
 348 |      if (::ChainstateActive().IsInitialBlockDownload())
 349 |          return false;
 350 | -    if (::ChainActive().Tip()->GetBlockTime() < (GetTime() - MAX_FEE_ESTIMATION_TIP_AGE))
 351 | +    if (::ChainActive().Tip()->GetBlockTime() < (GetTime<std::chrono::seconds>() - MAX_FEE_ESTIMATION_TIP_AGE).count())
```
MarcoFalke commented at 10:21 AM on May 28, 2020:

if (::ChainActive().Tip()->GetBlockTime() < count_seconds(GetTime<std::chrono::seconds>() - MAX_FEE_ESTIMATION_TIP_AGE))

Otherwise .count will return the wrong value in a future refactor where the second operand of the - changes its type to something with greater precision than seconds.
fanquake commented at 1:54 PM on May 28, 2020:

Done. It'd be great if you could open an issue documenting what these future, higher-precision time refactor changes are going to be, so it's clearer why we are writing code to accommodate them now.

MarcoFalke commented at 2:11 PM on May 28, 2020:

There are no specific refactors planned. The mentioned refactor was imaginary to illustrate why writing code as robust as possible is important.

in src/validation.cpp:2314 in b661af29df outdated

2316 | -        bool fPeriodicWrite = mode == FlushStateMode::PERIODIC && nNow > nLastWrite + (int64_t)DATABASE_WRITE_INTERVAL * 1000000;
2317 | -        // It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage.
2318 | -        bool fPeriodicFlush = mode == FlushStateMode::PERIODIC && nNow > nLastFlush + (int64_t)DATABASE_FLUSH_INTERVAL * 1000000;
2319 | +        // It's been a while since we wrote the block index to disk. Do this hourly, so we don't need to redownload after a crash.
2320 | +        bool fPeriodicWrite = mode == FlushStateMode::PERIODIC && nNow > nLastWrite + DATABASE_WRITE_INTERVAL;
2321 | +        // It's been very long since we flushed the cache. Do this daily, to optimize cache usage.

MarcoFalke commented at 10:23 AM on May 28, 2020:

style-nit only: I think the comment should stay as is before, otherwise it will be a pain to change it again should the value change from 1 day to let's say 23 hours.

fanquake commented at 1:54 PM on May 28, 2020:

Reverted.

MarcoFalke approved
MarcoFalke commented at 10:23 AM on May 28, 2020: member

Concept ACK
MarcoFalke added the label Refactoring on May 28, 2020
laanwj commented at 10:34 AM on May 28, 2020: member

Concept ACK, I think this makes code dealing with time more readable and type-safe.
validation: use std::chrono in CChainState::FlushStateToDisk() 47be28c8bc
validation: use std::chrono in IsCurrentForFeeEstimation() 789e9dd3aa
fanquake force-pushed on May 28, 2020
MarcoFalke commented at 2:13 PM on May 28, 2020: member

ACK 789e9dd3aa727176797529c35b2848f994630a82
DrahtBot commented at 10:10 PM on May 30, 2020: 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:
- #19005 (doc: Add documentation for 'checklevel' argument in 'verifychain' RPC… by kcalvinalvin)
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 requested review from laanwj on Jun 2, 2020
jonatack commented at 5:08 PM on June 3, 2020: member

ACK 789e9dd3aa727176797529c35b2848f994630a82
MarcoFalke merged this on Jun 3, 2020
MarcoFalke closed this on Jun 3, 2020
sidhujag referenced this in commit c71dcd461e on Jun 3, 2020
fanquake deleted the branch on Jun 4, 2020
fanquake removed review request from laanwj on Jun 4, 2020
Fabcien referenced this in commit 089a3050c3 on Feb 24, 2021
PastaPastaPasta referenced this in commit 60b4778a7b on Nov 1, 2021
PastaPastaPasta referenced this in commit 3148749e36 on Nov 1, 2021
PastaPastaPasta referenced this in commit bdb1a55c0b on Nov 3, 2021
pravblockc referenced this in commit d9c115811d on Nov 18, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Refactoring Validation

validation: use std::chrono throughout some validation functions #19088

Conflicts