validation: fix broken maxtipage comparison #26477

jamesob · 2022-11-09T17:10:41Z

Since faf4487, the maxtipage comparison in IsInitialBlockDownload() has been broken, since the NodeClock::now() time_point is in the system's native denomination (nanoseconds).

Without this patch, specifying the maximum allowable -maxtipage (9223372036854775807) results in a SIGABRT crash:

% gdb --args ./src/bitcoind -maxtipage=9223372036854775207 -minimumchainwork=0x00 -stopatheight=30000
...
2022-11-09T15:55:17Z [dnsseed] dnsseed thread exit
[Thread 0x7fff937fe640 (LWP 69883) exited]

Thread 29 "b-msghand" received signal SIGABRT, Aborted.
[Switching to Thread 0x7fff91ffb640 (LWP 69886)]
__pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
44      ./nptl/pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
#1  0x00007ffff768989f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  0x00007ffff763da52 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007ffff7628469 in __GI_abort () at ./stdlib/abort.c:79
#4  0x00007ffff7cf79a4 in __mulvdi3 () from /lib/x86_64-linux-gnu/libgcc_s.so.1
#5  0x00005555558d13ab in std::chrono::__duration_cast_impl<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, std::ratio<1000000000l, 1l>, long, false, true>::__cast<long, std::ratio<1l, 1l> > (__d=...) at /usr/include/c++/12/bits/chrono.h:521
#6  std::chrono::duration_cast<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__d=...)
    at /usr/include/c++/12/bits/chrono.h:260
#7  std::chrono::duration<long, std::ratio<1l, 1000000000l> >::duration<long, std::ratio<1l, 1l>, void> (__d=..., this=<optimized out>)
    at /usr/include/c++/12/bits/chrono.h:514
#8  std::chrono::operator-<long, std::ratio<1l, 1000000000l>, long, std::ratio<1l, 1l> > (__rhs=..., __lhs=...)
    at /usr/include/c++/12/bits/chrono.h:650
#9  std::chrono::operator-<NodeClock, std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__rhs=...,
    __lhs=...) at /usr/include/c++/12/bits/chrono.h:1020
#10 Chainstate::IsInitialBlockDownload (this=0x555556071940) at ./src/validation.cpp:1545
#11 0x00005555556efd1e in operator() (__closure=<optimized out>) at ./src/net_processing.cpp:3369
#12 (anonymous namespace)::PeerManagerImpl::ProcessMessage (this=0x555556219be0, pfrom=..., msg_type=..., vRecv=..., time_received=...,
    interruptMsgProc=...) at ./src/net_processing.cpp:3369
#13 0x00005555556f75cc in (anonymous namespace)::PeerManagerImpl::ProcessMessages (this=0x555556219be0, pfrom=<optimized out>,
    interruptMsgProc=std::atomic<bool> = { false }) at ./src/net_processing.cpp:4985
#14 0x00005555556a83c9 in CConnman::ThreadMessageHandler (this=0x5555560ebc70) at ./src/net.cpp:2014
#15 0x0000555555c4d5d6 in std::function<void ()>::operator()() const (this=0x7fff91ffadb0) at /usr/include/c++/12/bits/std_function.h:591
#16 util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>) (
    thread_name="0\255\377\221\377\177\000\000\v\000\000\000\000\000\000\000TraceThread\000\000\000\000\000P\255\377\221\377\177\000\000\017\000\000\000\000\000\000\000util/thread.cpp\000\000\000\000\000\000\000\000\000\000ihB鵿6\000\000\000\000\000\000\000\000\260\255\377\221\377\177\000\000\277\211\321UUU\000\000p\324\304UUU\000\000\002\000\000\000\000\000\000\000\240xh\367\377\177\000\000\000\000\000\000\000\000\000\000]\340iUUU\000\000p\274\016VUU\000\000\000\000\000\000\000\000\000\000\300\303iUUU\000\000p\206jUUU", '\000' <repeats 11 times>, "ihB鵿6\200\251!VUU\000\000"..., thread_func=...) at util/thread.cpp:21
#17 0x000055555569e05d in std::__invoke_impl<void, void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__f=<optimized out>) at /usr/include/c++/12/bits/invoke.h:61
#18 std::__invoke<void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__fn=<optimized out>) at /usr/include/c++/12/bits/invoke.h:96
#19 std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::_M_invoke<0, 1, 2> (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:252
#20 std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::operator() (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:259
#21 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > > >::_M_run(void) (this=<optimized out>)
    at /usr/include/c++/12/bits/std_thread.h:210
#22 0x00007ffff7ad43d3 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#23 0x00007ffff7687b27 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:435
#24 0x00007ffff770a78c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
(gdb)

jamesob · 2022-11-09T19:01:06Z

test/functional/test_framework/util.py

+    timeout_float = float(timeout if timeout is not None else 'inf')
+    if attempts == float('inf') and timeout_float == float('inf'):
+        timeout_float = 60
+    timeout_float = timeout_float * timeout_factor


All this churn here is because of the mypy linter. I tried assert isinstance(timeout, float) and assert timeout is not None to no avail.

if mypy is broken, wouldn't it be better to not add the annotation, as opposed to changing the code?

It's not the annotation that's broken; it's the fact that in order to specify a timeout to connect_nodes, we have to change the default parameter here from a float to None, which then makes mypy confused.

maflcko

Not sure about this fix. The whole point of std::chrono is that the programmer does not have to do verbose manual conversions. Instead the compiler injects the multiplication only where needed.

Generally, the underlying issue is that int settings are not sanitized. This affects almost all int settings. See for example #21893 (comment) or #25561 (comment)

As for the fix, this reminds me a bit of 290ff5e, so I think what should really be done is some kind of saturating parser.

Another reason why this should be done in the parser is that it doesn't really make sense to accept durations that are longer than 50 times the average lifespan of a star.

jamesob · 2022-11-10T13:38:12Z

As for the fix, this reminds me a bit of 290ff5e, so I think what should really be done is some kind of saturating parser.

This sounds less simple and immediately accessible than the one-line fix here.

Right now master is broken; for usages that worked prior to faf4487, bitcoind now crashes. IMO the priority should be addressing that and then moving on to less immediate issues, like what the aesthetically preferable fix should be.

I agree with you that probably some more substantial change is needed to avoid similar issues in the future, but this change is probably the simplest thing that could be done to unbreak master, and it introduces test coverage.

In the spirit of forward progress (and not having a broken master!), I would recommend that either we merge this as-is or you propose an alternative in short order, because I have other stuff to do in the immediate term besides rethink integer parsing across the codebase, and I'd like to e.g. be able to rebase assumeutxo on master and test it without crashing.

maflcko

lgtm, the validation.cpp changes are fine, haven't looked at the tests.

Though, if you need an immediate fix, I'd recommend to temporarily replace the value in your config with a smaller, but large enough value.

Bitcoin currently does not support timestamps after year 2106, so using a duration that puts you there should be fine for your needs.

>>> 2**32 - 1
4294967295

I am happy to merge this instead, if reviewers prefer.

maflcko

Left some nits, feel free to ignore

src/validation.cpp

test/functional/feature_maxtipage.py

maflcko · 2022-11-11T09:07:30Z

For reference, I tried to fix the overflow via ccccee0, but this only fixes it for large values, not small ones. The pull here also fixes it only for large ones, which is fine, because underflow was the behaviour previously as well.

maflcko · 2022-11-11T16:01:01Z

test/functional/test_framework/util.py

+    timeout_float = float(timeout if timeout is not None else 'inf')
+    if attempts == float('inf') and timeout_float == float('inf'):
+        timeout_float = 60
+    timeout_float = timeout_float * timeout_factor


if mypy is broken, wouldn't it be better to not add the annotation, as opposed to changing the code?

test/functional/test_framework/test_framework.py

test/functional/feature_maxtipage.py

DrahtBot · 2022-11-12T00:51:35Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke

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

Conflicts

No conflicts as of last run.

Since faf4487, the maxtipage comparison in IsInitialBlockDownload() has been broken, since the NodeClock::now() time_point is in the system's native denomination (micrcoseconds). Without this patch, specifying the maximum allowable -maxtipage (9223372036854775807) results in a SIGABRT crash. Co-authored-by: MacroFake <falke.marco@gmail.com>

jamesob · 2022-11-14T14:54:41Z

Even though I think it's probably nice to be able to specify a timeout for connect_nodes, I've removed that commit from this PR to avoid more bikeshedding.

maflcko

lgtm ACK 8eafa99

test/functional/feature_maxtipage.py

jamesob · 2022-11-30T20:37:27Z

More reviewers needed or RFM?

maflcko

review ACK e4be0e9 🏽

Show signature

Signature:

e4be0e9 test: add -maxtipage test for the maximum allowable value (James O'Beirne) a451e83 fix: validation: cast now() to seconds for maxtipage comparison (James O'Beirne) Pull request description: Since bitcoin@faf4487, the maxtipage comparison in IsInitialBlockDownload() has been broken, since the NodeClock::now() time_point is in the system's native denomination (nanoseconds). Without this patch, specifying the maximum allowable -maxtipage (9223372036854775807) results in a SIGABRT crash: ``` % gdb --args ./src/bitcoind -maxtipage=9223372036854775207 -minimumchainwork=0x00 -stopatheight=30000 ... 2022-11-09T15:55:17Z [dnsseed] dnsseed thread exit [Thread 0x7fff937fe640 (LWP 69883) exited] Thread 29 "b-msghand" received signal SIGABRT, Aborted. [Switching to Thread 0x7fff91ffb640 (LWP 69886)] __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44 44 ./nptl/pthread_kill.c: No such file or directory. (gdb) bt #0 __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44 #1 0x00007ffff768989f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78 #2 0x00007ffff763da52 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #3 0x00007ffff7628469 in __GI_abort () at ./stdlib/abort.c:79 #4 0x00007ffff7cf79a4 in __mulvdi3 () from /lib/x86_64-linux-gnu/libgcc_s.so.1 #5 0x00005555558d13ab in std::chrono::__duration_cast_impl<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, std::ratio<1000000000l, 1l>, long, false, true>::__cast<long, std::ratio<1l, 1l> > (__d=...) at /usr/include/c++/12/bits/chrono.h:521 #6 std::chrono::duration_cast<std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__d=...) at /usr/include/c++/12/bits/chrono.h:260 #7 std::chrono::duration<long, std::ratio<1l, 1000000000l> >::duration<long, std::ratio<1l, 1l>, void> (__d=..., this=<optimized out>) at /usr/include/c++/12/bits/chrono.h:514 #8 std::chrono::operator-<long, std::ratio<1l, 1000000000l>, long, std::ratio<1l, 1l> > (__rhs=..., __lhs=...) at /usr/include/c++/12/bits/chrono.h:650 #9 std::chrono::operator-<NodeClock, std::chrono::duration<long, std::ratio<1l, 1000000000l> >, long, std::ratio<1l, 1l> > (__rhs=..., __lhs=...) at /usr/include/c++/12/bits/chrono.h:1020 #10 Chainstate::IsInitialBlockDownload (this=0x555556071940) at ./src/validation.cpp:1545 #11 0x00005555556efd1e in operator() (__closure=<optimized out>) at ./src/net_processing.cpp:3369 #12 (anonymous namespace)::PeerManagerImpl::ProcessMessage (this=0x555556219be0, pfrom=..., msg_type=..., vRecv=..., time_received=..., interruptMsgProc=...) at ./src/net_processing.cpp:3369 #13 0x00005555556f75cc in (anonymous namespace)::PeerManagerImpl::ProcessMessages (this=0x555556219be0, pfrom=<optimized out>, interruptMsgProc=std::atomic<bool> = { false }) at ./src/net_processing.cpp:4985 #14 0x00005555556a83c9 in CConnman::ThreadMessageHandler (this=0x5555560ebc70) at ./src/net.cpp:2014 #15 0x0000555555c4d5d6 in std::function<void ()>::operator()() const (this=0x7fff91ffadb0) at /usr/include/c++/12/bits/std_function.h:591 #16 util::TraceThread(std::basic_string_view<char, std::char_traits<char> >, std::function<void ()>) ( thread_name="0\255\377\221\377\177\000\000\v\000\000\000\000\000\000\000TraceThread\000\000\000\000\000P\255\377\221\377\177\000\000\017\000\000\000\000\000\000\000util/thread.cpp\000\000\000\000\000\000\000\000\000\000ihB鵿6\000\000\000\000\000\000\000\000\260\255\377\221\377\177\000\000\277\211\321UUU\000\000p\324\304UUU\000\000\002\000\000\000\000\000\000\000\240xh\367\377\177\000\000\000\000\000\000\000\000\000\000]\340iUUU\000\000p\274\016VUU\000\000\000\000\000\000\000\000\000\000\300\303iUUU\000\000p\206jUUU", '\000' <repeats 11 times>, "ihB鵿6\200\251!VUU\000\000"..., thread_func=...) at util/thread.cpp:21 #17 0x000055555569e05d in std::__invoke_impl<void, void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__f=<optimized out>) at /usr/include/c++/12/bits/invoke.h:61 #18 std::__invoke<void (*)(std::basic_string_view<char>, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > (__fn=<optimized out>) at /usr/include/c++/12/bits/invoke.h:96 #19 std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::_M_invoke<0, 1, 2> (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:252 #20 std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > >::operator() (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:259 #21 std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(std::basic_string_view<char, std::char_traits<char> >, std::function<void()>), char const*, CConnman::Start(CScheduler&, const Options&)::<lambda()> > > >::_M_run(void) (this=<optimized out>) at /usr/include/c++/12/bits/std_thread.h:210 #22 0x00007ffff7ad43d3 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6 #23 0x00007ffff7687b27 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:435 #24 0x00007ffff770a78c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 (gdb) ``` ACKs for top commit: MarcoFalke: review ACK e4be0e9 🏽 Tree-SHA512: d892d6264a284d952a68a8631a6301277373b8df939dafd9e2652f2f22ab60712cde63b90c27c67ea2d05f02443452e3e4e1b9f25479bfaca00d4c4de13b9fbd

jamesob force-pushed the 2022-11-fix-maxtipage branch from 94b4b67 to 9b31b60 Compare November 9, 2022 17:23

DrahtBot added the Validation label Nov 9, 2022

jamesob force-pushed the 2022-11-fix-maxtipage branch from 9b31b60 to b3a7afc Compare November 9, 2022 18:50

jamesob mentioned this pull request Nov 9, 2022

refactor: Remove almost all validation option globals #25704

Merged

jamesob commented Nov 9, 2022

View reviewed changes

maflcko suggested changes Nov 10, 2022

View reviewed changes

maflcko added this to the 25.0 milestone Nov 10, 2022

maflcko approved these changes Nov 10, 2022

View reviewed changes

maflcko reviewed Nov 10, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

test/functional/feature_maxtipage.py Outdated Show resolved Hide resolved

test/functional/feature_maxtipage.py Outdated Show resolved Hide resolved

jamesob force-pushed the 2022-11-fix-maxtipage branch 2 times, most recently from 06cec97 to 35ef2bf Compare November 11, 2022 15:34

maflcko reviewed Nov 11, 2022

View reviewed changes

This was referenced Nov 13, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

assumeutxo #15606

Closed

jamesob force-pushed the 2022-11-fix-maxtipage branch from 35ef2bf to 8eafa99 Compare November 14, 2022 14:46

maflcko approved these changes Nov 14, 2022

View reviewed changes

test/functional/feature_maxtipage.py Outdated Show resolved Hide resolved

jamesob force-pushed the 2022-11-fix-maxtipage branch from 8eafa99 to e4be0e9 Compare November 14, 2022 15:33

test: add -maxtipage test for the maximum allowable value

e4be0e9

maflcko reviewed Dec 12, 2022

View reviewed changes

fanquake merged commit 968f03e into bitcoin:master Dec 13, 2022

bitcoin locked and limited conversation to collaborators Dec 13, 2023

validation: fix broken maxtipage comparison #26477

validation: fix broken maxtipage comparison #26477

Uh oh!

Conversation

jamesob commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

jamesob Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob commented Nov 10, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 11, 2022

Uh oh!

maflcko Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Nov 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

jamesob commented Nov 14, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Nov 30, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Nov 9, 2022 •

edited

Loading

jamesob Nov 14, 2022 •

edited

Loading

maflcko left a comment •

edited

Loading

DrahtBot commented Nov 12, 2022 •

edited

Loading