rpc, bugfix: Enforce maximum value for setmocktime #29869

1. 
   dergoegge commented at 8:02 am on April 15, 2024: member

   The maximum value for our mocktime must be representable in nanoseconds, otherwise we end up with negative values returned from NodeClock::now().

   Found through fuzzing:

   0$ echo "c2V0bW9ja3RpbWVcZTptYf9w/3NldG3///////////////9p////ZP///ymL//////89////Nv9L////////LXkBAABpAA==" | base64 --decode > rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
   1$ FUZZ=rpc ./src/test/fuzz/fuzz rpc-8cab9148ab4418ebd1923c213e9d3fe9c9b49b39.crash
   2fuzz_libfuzzer: util/time.cpp:28: static NodeClock::time_point NodeClock::now(): Assertion `ret > 0s' failed.
   

2. 
   DrahtBot commented at 8:02 am on April 15, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	maflcko, brunoerg, glozow

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. in src/rpc/node.cpp:63 in 3740a63b15 outdated

   61@@ -61,6 +62,12 @@ static RPCHelpMan setmocktime()
   62     if (time < 0) {
   63         throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime cannot be negative: %s.", time));
   

   ---

   ---

   

   maflcko commented at 8:17 am on April 15, 2024:

   0        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Mocktime must be in the range [0, %s], not %s.", max_time, time));
   

   ---

   

   dergoegge commented at 8:20 am on April 15, 2024:

   Done.
4. maflcko approved
5. 
   maflcko commented at 8:18 am on April 15, 2024: member

   Could combine the two error messages into one, as both are related to out-of-range?
6. dergoegge force-pushed on Apr 15, 2024
7. in src/rpc/node.cpp:30 in f474a1a50c outdated

   26@@ -27,6 +27,7 @@
   27 #include <util/any.h>
   28 #include <util/check.h>
   29 
   30+#include <chrono>
   

   ---

   ---

   

   maflcko commented at 8:21 am on April 15, 2024:

   nit: could be better to remove this and use util/time.h instead?

   ---

   

   dergoegge commented at 8:23 am on April 15, 2024:

   Done
8. maflcko approved
9. 
   maflcko commented at 8:21 am on April 15, 2024: member

   Thanks, lgtm
10. dergoegge force-pushed on Apr 15, 2024
11. 
    maflcko commented at 8:27 am on April 15, 2024: member

    lgtm ACK 61df72ee4dee3c5d739a3c851c86fdb4e4c640e5
12. maflcko added the label Tests on Apr 15, 2024
13. maflcko removed the label Tests on Apr 15, 2024
14. [rpc, bugfix] Enforce maximum value for setmocktime c2e0489b71
15. dergoegge force-pushed on Apr 15, 2024
16. DrahtBot added the label CI failed on Apr 15, 2024
17. 
    dergoegge commented at 8:51 am on April 15, 2024: member

    Fixed rpc_uptime functional test
18. 
    maflcko commented at 9:00 am on April 15, 2024: member

    re-ACK c2e0489b7125cceaeef355fc274dd8988822fff4
19. brunoerg approved
20. 
    brunoerg commented at 9:40 am on April 15, 2024: contributor

    crACK c2e0489b7125cceaeef355fc274dd8988822fff4
21. in test/functional/rpc_uptime.py:26 in c2e0489b71

    22@@ -23,7 +23,7 @@ def run_test(self):
    23         self._test_uptime()
    24 
    25     def _test_negative_time(self):
    26-        assert_raises_rpc_error(-8, "Mocktime cannot be negative: -1.", self.nodes[0].setmocktime, -1)
    27+        assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not -1.", self.nodes[0].setmocktime, -1)
    

    ---

    ---

    

    glozow commented at 10:19 am on April 15, 2024:

    Maybe add a

    0assert_raises_rpc_error(-8, "Mocktime must be in the range [0, 9223372036], not 9223372037.", self.nodes[0].setmocktime, 9223372037)
    

    ?

    ---

    

    maflcko commented at 11:22 am on April 15, 2024:

    If you are changing this, it could also make sense to test that a full round trip of the value 9223372036 works.
22. 
    glozow commented at 10:20 am on April 15, 2024: member

    ACK c2e0489b7125cceaeef355fc274dd8988822fff4
23. DrahtBot removed the label CI failed on Apr 15, 2024
24. glozow merged this on Apr 15, 2024
25. glozow closed this on Apr 15, 2024

    ---

26. fanquake referenced this in commit 0c34f12e06 on Apr 16, 2024
27. fanquake referenced this in commit 897e5af58a on Apr 16, 2024
28. 
    fanquake commented at 7:59 am on April 17, 2024: member

    Backported to 27.x in #29888.
29. glozow referenced this in commit a81a9228fb on Apr 17, 2024
30. fanquake referenced this in commit c7885ecd77 on May 13, 2024
31. 
    glozow commented at 2:27 pm on May 13, 2024: member

    Backport for 26.x is in #29899

Contributors
dergoegge DrahtBot maflcko brunoerg glozow fanquake