doc: unify unix epoch time descriptions #17617

jonatack commented at 8:52 PM on November 26, 2019: member

Updated call sites: mocktime, getblockheader, getblock, pruneblockchain, getchaintxstats, getblocktemplate, setmocktime, getpeerinfo, setban, getnodeaddresses, getrawtransaction, importmulti, listtransactions, listsinceblock, gettransaction, getwalletinfo, getaddressinfo

Commands for testing manually:

bitcoind -help-debug | grep -A1 mocktime
bitcoin-cli help getblockheader
bitcoin-cli help getblock
bitcoin-cli help pruneblockchain
bitcoin-cli help getchaintxstats
bitcoin-cli help getblocktemplate
bitcoin-cli help setmocktime
bitcoin-cli help getpeerinfo
bitcoin-cli help setban
bitcoin-cli help getnodeaddresses
bitcoin-cli help getrawtransaction
bitcoin-cli help importmulti
bitcoin-cli help listtransactions
bitcoin-cli help listsinceblock
bitcoin-cli help gettransaction
bitcoin-cli help getwalletinfo
bitcoin-cli help getaddressinfo

fanquake added the label RPC/REST/ZMQ on Nov 26, 2019

jonatack renamed this:
~~rpc: replace varying UNIX time strings with a constant~~
doc: unify unix epoch time description in rpc output
on Nov 26, 2019

jonatack renamed this:
~~doc: unify unix epoch time description in rpc output~~
qa: unify unix epoch time descriptions
on Nov 26, 2019

jonatack force-pushed on Nov 26, 2019

jonatack commented at 9:06 PM on November 26, 2019: member

Renamed the commit.

fanquake removed the label RPC/REST/ZMQ on Nov 26, 2019

fanquake added the label Docs on Nov 26, 2019

fanquake added the label RPC/REST/ZMQ on Nov 26, 2019

practicalswift commented at 9:41 PM on November 26, 2019: contributor

Concept ACK: consistency is nice

DrahtBot commented at 1:26 AM on November 27, 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:

#17585 (rpc: deprecate getaddressinfo label by jonatack)
#17578 (rpc: simplify getaddressinfo labels, deprecate previous behavior by jonatack)

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.

in src/rpc/util.h:25 in b8e253d376 outdated

  21 | @@ -22,6 +22,8 @@
  22 |  
  23 |  #include <boost/variant.hpp>
  24 |  
  25 | +static const std::string UNIX_EPOCH_TIME = "UNIX epoch time";

laanwj commented at 8:58 AM on November 27, 2019:

needs a comment

/** String used to describe UNIX epoch times in documentation, factored out to a constant for consistency. */

or something

laanwj commented at 9:05 AM on November 27, 2019:

I also think defining the value of a string constant in the header can give linker issues in C++, or at least increases executable size because this will be in every compilation unit.

usually you'd do in util.h:

extern const std::string UNIX_EPOCH_TIME;

the in util.cpp

const std::string UNIX_EPOCH_TIME = "UNIX epoch time";

jonatack commented at 9:23 AM on November 27, 2019:

Thanks! Done, LMK if it would be better to comment the header rather than the definition.

laanwj commented at 9:05 AM on November 27, 2019: member

Concept ACK

jonatack force-pushed on Nov 27, 2019

jonatack commented at 10:45 AM on November 27, 2019: member

The lone Travis CI failure appears to be an unrelated one-off DOCKER_EXEC apt-get install issue.

laanwj commented at 12:16 PM on November 27, 2019: member

The lone Travis CI failure appears to be an unrelated one-off DOCKER_EXEC apt-get install issue.

Yes, apt has been flaky on Travis lately. Either it takes way too long, or doesn't work at all. See also #17546 (comment).

Anyhow, restarted the build.

in src/rpc/util.cpp:16 in 42a1e47b8d outdated

  12 | @@ -13,6 +13,12 @@
  13 |  
  14 |  #include <tuple>
  15 |  
  16 | +/**
  17 | + * String used to describe UNIX epoch time in documentation, factored out to a
  18 | + * constant for consistency.
  19 | + */
  20 | +const std::string UNIX_EPOCH_TIME = "UNIX epoch time";

kristapsk commented at 6:34 PM on December 1, 2019:

It is kinda common knowledge, but wouldn't something like "UNIX epoch time (seconds since Jan 1 1970 GMT)" still be better?

jonatack commented at 6:14 AM on December 2, 2019:

See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-26.html#l-368 to 373.

laanwj commented at 2:31 PM on December 3, 2019:

the motivation here was: it is not within the domain of our help to explain what is meant here various programming languages provide APIs that use epoch times, that's why it's used here, most developers will be better off treating it as an opaque value instead of trying to roll their own time handling (and if someone really want to know what it is, that's great, but it's easy enough to find information on Wikipedia and such)

jonatack commented at 5:10 PM on December 3, 2019:

Yes. I probably should have mentioned that I agree with this motivation which is why the PR is proposed this way... hmmm it may be good to explain this in the "RPC interface guidelines" section of doc/developer-notes.md

laanwj commented at 5:16 PM on December 3, 2019:

extending scope is easy, decreasing scope is always an uphill battle :smile:

jonatack commented at 5:41 PM on December 3, 2019:

attempted an explanation at the bottom of developer-notes.md

in doc/developer-notes.md:1055 in 7614522c7f outdated

1051 | +- Use the `UNIX_EPOCH_TIME` constant when describing Unix epoch time or
1052 | +  timestamps in the RPC documentation.
1053 | +
1054 | +  - *Rationale*: User-facing consistency. Note that all times in Bitcoin are not
1055 | +    clocks (e.g. C++11 time points) but durations expressed in Unix epoch time
1056 | +    (e.g. in seconds since the epoch Jan 1 1970 GMT).

laanwj commented at 10:18 AM on December 5, 2019:

(except that leap seconds are ignored, but I don't think this is worth mentioning here :smile:)

jonatack commented at 12:32 PM on December 5, 2019:

(credit to @MarcoFalke here for the "Note" part)

MarcoFalke commented at 7:12 PM on December 12, 2019:

On a second thought, while this is probably correct, it might be more confusing that helpful. A time_point is templated on clock and duration ( https://en.cppreference.com/w/cpp/chrono/time_point ). And assuming a C++20 system_clock, that duration is counted from unix time 0.

MarcoFalke commented at 7:12 PM on December 12, 2019:

So I'd suggest to just keep the rationale at "user-facing consistency"

laanwj commented at 10:25 AM on December 5, 2019: member

ACK 42a1e47b8d4e8edc1e4c3f43eea3cf0f36476b42 re-ACK d94d34f05f4ae3efa07de409489d68bbcc216346 I have reviewed the code and checked the new description. I have not investigated completeness, e.g. if these are all occurrences of times on the RPC interface.

laanwj renamed this:
~~qa: unify unix epoch time descriptions~~
doc: unify unix epoch time descriptions
on Dec 12, 2019

laanwj commented at 10:58 AM on December 12, 2019: member

Changed qa: to doc: as the main change here is a (RPC) documentation change

in src/rpc/util.cpp:19 in 7614522c7f outdated

  12 | @@ -13,6 +13,12 @@
  13 |  
  14 |  #include <tuple>
  15 |  
  16 | +/**
  17 | + * String used to describe UNIX epoch time in documentation, factored out to a
  18 | + * constant for consistency.
  19 | + */

MarcoFalke commented at 7:06 PM on December 12, 2019:

doc goes into header, usually

jonatack commented at 1:07 AM on December 13, 2019:

moved to header

MarcoFalke approved

MarcoFalke commented at 7:12 PM on December 12, 2019: member

ACK 7614522c7f15f9e4dcb9e951d1a274b76178d24e 🏖

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 50c4c0a244138ca61874fad53230f7bb0d2d390404ae403127b8515ac87079b7 -

</details>

qa: unify unix epoch time descriptions

to "UNIX epoch time".

Call sites updated:
```
mocktime
getblockheader
getblock
pruneblockchain
getchaintxstats
getblocktemplate
setmocktime
getpeerinfo
setban
getnodeaddresses
getrawtransaction
importmulti
listtransactions
listsinceblock
gettransaction
getwalletinfo
getaddressinfo
```

e2f32cb5c5

doc: update developer notes wrt unix epoch time d94d34f05f

jonatack force-pushed on Dec 13, 2019

jonatack commented at 1:08 AM on December 13, 2019: member

Updated as per @MarcoFalke's suggestions.

laanwj referenced this in commit d4b335c60a on Dec 13, 2019

laanwj merged this on Dec 13, 2019

laanwj closed this on Dec 13, 2019

jonatack deleted the branch on Dec 13, 2019

jasonbcox referenced this in commit 807ed300fa on Oct 2, 2020

DrahtBot locked this on Dec 16, 2021