Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[0.19] scheduler: Workaround negative nsecs bug in boost's wait_until #18284

Merged
merged 1 commit into from Aug 28, 2020

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Mar 6, 2020

Some boost versions have a bug that can cause a time prior to system boot (or wake from sleep) to throw an exception instead of return timeout
See boostorg/thread#308

NOTE: This was addressed in master with a refactor (#18234), so this isn't a strict backport and needs full review.

Fixes #18227

Cleanly merges to 0.14+

Some boost versions have a bug that can cause a time prior to system boot (or wake from sleep) to throw an exception instead of return timeout
See boostorg/thread#308
@laanwj laanwj added this to the 0.19.2 milestone Mar 6, 2020
@maflcko
Copy link
Member

maflcko commented Mar 6, 2020

NOTE: This was addressed in master with a refactor (#18234), so this isn't a strict backport and needs full review.

If we fix this, I'd prefer to just backport that refactor

@ajtowns
Copy link
Contributor

ajtowns commented Mar 7, 2020

Backporting the refactor gets conflicts from the mockforward and uninterruptiblesleep patches at least -- I've had a go at https://github.com/ajtowns/bitcoin/commits/202003-scheduler-deboost-019 if anyone wants to look into it further. I think it's worth at least considering this much simpler approach if we want to improve 0.19 or earlier here. So Concept ACK I guess.

@laanwj
Copy link
Member

laanwj commented Mar 12, 2020

I think this fix is pretty clear and minimal. Backporting complicated refactors sounds more risky.
ACK ed0223e

@gruve-p
Copy link
Contributor

gruve-p commented Apr 8, 2020

ACK ed0223e

@maflcko
Copy link
Member

maflcko commented Aug 27, 2020

I think both backporting the fix we did on master or coming up with a new fix are risky.

Taking into account that this is not a regression with 0.19, that this doesn't happen on server, but only laptops (or machines that supsend/hibernate), it seems low risk to keep the bug as is.

Laptop users might also be faster to upgrade to 0.20, where this is already fixed.

@maflcko
Copy link
Member

maflcko commented Aug 27, 2020

So weak NACK from me

@maflcko
Copy link
Member

maflcko commented Aug 27, 2020

Though, it also has two ACKs, so it might be good.

Has this been previously released in Knots and tested?

@luke-jr
Copy link
Member Author

luke-jr commented Aug 27, 2020

Yes, it was released in Knots 0.19.1.knots20200304, which had ~3 months before being replaced by 0.20.0. Not sure what it peaked at, but there's approx 25 nodes still running it today.

@maflcko
Copy link
Member

maflcko commented Aug 28, 2020

Ok, then I am merging this with two ACKs and because it was tested in production. I have not reviewed the code myself.

@maflcko maflcko merged commit 89a6bb9 into bitcoin:0.19 Aug 28, 2020
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 8, 2020
ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Dec 1, 2020
Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>

Summary
---

This fixes issue Bitcoin-ABC#180 and also re-enables the mockforward test disabled
by !837.

It is a backport from core with a workaround to boost bug:
boostorg/thread#308, which was the source of
the mockforward tests failing in Bitcoin-ABC#180.

Original core PR: bitcoin/bitcoin#18284

Original Core Commit Message
---

    Some boost versions have a bug that can cause a time prior to system boot
    (or wake from sleep) to throw an exception instead of return timeout
    See boostorg/thread#308

    NOTE: This was addressed in master with a refactor (#18234), so this isn't
    a strict backport and needs full review.

    Fixes #18227

    Cleanly merges to 0.14+

Test Plan
---

- `ninja check`

Advanced:

- If you can, try and get the mockforward test to fail without this
  fix, and then try and get it to fail again with this fix.  If it
  succeeds always with this fix we are golden.  The test is in
  "scheduled_tests".
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants