init: Fixes for file descriptor accounting #27539

pull Empact wants to merge 4 commits into bitcoin:master from Empact:2023-04-minimum-file-descriptor-18911 changing 1 files +10 −11
  1. Empact commented at 6:33 pm on April 28, 2023: member

    This rebases and revises #16003 for clarity of review.

    Aims to fix #18911.

  2. DrahtBot commented at 6:33 pm on April 28, 2023: 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
    Concept ACK fjahr, TheCharlatan

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

    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.

  3. Empact force-pushed on Apr 28, 2023
  4. DrahtBot added the label CI failed on Apr 28, 2023
  5. Empact force-pushed on Apr 28, 2023
  6. DrahtBot removed the label CI failed on Apr 28, 2023
  7. glozow added the label Docs on May 8, 2023
  8. in src/init.cpp:981 in bbad166e42 outdated
    935+    }
    936 
    937-#ifdef USE_POLL
    938-    int fd_max = nFD;
    939-#else
    940-    int fd_max = FD_SETSIZE;
    


    fjahr commented at 8:14 am on May 24, 2023:
    Needs an explanation of why it is ok to take FD_SETSIZE completely out of the equation in the commit description.
  9. fjahr commented at 8:17 am on May 24, 2023: contributor

    Concept ACK

    I think it would be very helpful to add comments to explain what is going on. See my accidentally duplicate PR for some suggestions: #27730.

  10. achow101 requested review from TheCharlatan on Sep 20, 2023
  11. DrahtBot added the label CI failed on Sep 29, 2023
  12. DrahtBot removed the label CI failed on Oct 4, 2023
  13. TheCharlatan commented at 12:06 pm on October 24, 2023: contributor

    Concept ACK

    First three commits look good, but the last one needs more descriptions and comments as mentioned by fjahr.

  14. DrahtBot added the label CI failed on Oct 25, 2023
  15. DrahtBot commented at 2:34 pm on February 5, 2024: contributor

    🤔 There hasn’t been much activity lately and the CI seems to be failing.

    If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

  16. refactor: constantize the unchanging nBind 446818c8c2
  17. refactor: Extract RESERVED_FILE_DESCRIPTORS constant 8eefee0984
  18. Log the file descriptor stats on init
    Co-authored-by: tryphe <tryphe@noreply.github.com>
    bf3369995e
  19. init: reserve file descriptors for bind() and accept()
    And fix the amount of descriptors requested
    
    Co-authored-by: tryphe <tryphe@noreply.github.com>
    44872b32bc
  20. Empact force-pushed on Mar 4, 2024
  21. DrahtBot removed the label CI failed on Mar 4, 2024
  22. TheCharlatan commented at 7:53 pm on May 1, 2024: contributor
    This was rebased fairly recently, but you have not addressed this open comment #27539 (review) . Are you still working on this?
  23. sr-gi commented at 5:39 pm on May 8, 2024: member
    I’ve been reviewing #29415 (which is extending the current brain-hurting approach on master), so I ended up considering fixing this. If @Empact is not working on it anymore I may pick it up
  24. fanquake commented at 8:10 am on May 15, 2024: member
    Closing for now, re #30065.
  25. fanquake closed this on May 15, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-29 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me