Page MenuHomePhabricator

[seeder] Bump thread stacksize
ClosedPublic

Authored by roqqit on Fri, Dec 13, 20:08.

Details

Reviewers
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC61b79c87cb21: [seeder] Bump thread stacksize
Summary

0x20000 is too small on some systems, causing pthread_create() to fail. It turns out that predicting the right value is not trivial, so we can just bump it slightly instead.

Depends on D17348, but only to discover the issue in the first place.

Test Plan

Supposedly setting ulimit -s or ulimit -Hs would allow you to reproduce, but this does not appear to work. If you have a borked environment that is enforcing large stack sizes, just run the seeder:

./bitcoin-seeder

Without the patch it can break. With the patch, it works.

Diff Detail

Event Timeline

Owners added a reviewer: Restricted Owners Package.Fri, Dec 13, 20:08
roqqit requested review of this revision.Fri, Dec 13, 20:08
roqqit planned changes to this revision.Fri, Dec 13, 20:08
roqqit requested review of this revision.Fri, Dec 13, 20:26
roqqit edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Mon, Dec 16, 09:15
Fabien added inline comments.
src/seeder/main.cpp
460 ↗(On Diff #51575)

We discussed this offline, I don't think this is doing what you expect, according to the doc (https://www.man7.org/linux/man-pages/man3/pthread_attr_setstacksize.3.html) the thread stack is not related to RLIMIT_STACK.

Also at the very least you need to check the return code from that function. It might just not work and you will never know.

This revision now requires changes to proceed.Mon, Dec 16, 09:15

Just bump the value because predicting it is proving to be difficult and time consuming

roqqit retitled this revision from [seeder] Respect resource limit for stacksize as set by the system to [seeder] Bump thread stacksize.Tue, Dec 17, 20:38
roqqit edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Tue, Dec 17, 20:46
This revision was automatically updated to reflect the committed changes.