makeseeds: fix off-by-one in field count check #36151

pull aman21-droid wants to merge 2 commits into bitcoin:master from aman21-droid:fix/makeseeds-field-count-off-by-one changing 1 files +22 −1
  1. aman21-droid commented at 5:05 PM on September 2, 2026: none

    Fixes #36146.

    parseline() checks that a line has at least 11 whitespace-separated fields:

    if len(sline) < 11:
        # line too short to be valid, skip it.
        return None
    

    but it later reads sline[11] (the user agent), which needs 12. A line with exactly 11 fields got past the check and then raised IndexError, aborting the whole run instead of skipping that line. 11 is the highest index the function uses, so the check now requires 12 fields.

    This isn't only theoretical: README.md builds seeds_main.txt by appending one crawler's output onto another's, so the file mixes two independently maintained formats, and a truncated download leaves a short last line too. Skipping the line is what the check was already trying to do.

    The first commit adds tests, since nothing covered makeseeds before. They pass on both commits with different expectations: it first records that an 11-field line raises IndexError, and the fix commit replaces that single assertion with the line being skipped. Run with:

    python3 -m unittest contrib.seeds.makeseeds
    
  2. DrahtBot added the label Scripts and tools on Sep 2, 2026
  3. DrahtBot commented at 5:06 PM on September 2, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36151.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK l0rinc

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. l0rinc commented at 7:00 PM on September 2, 2026: contributor

    The fix looks correct, but it would be slightly easier to review as a characterization plus fix pair, with the unit tests kept inline in makeseeds.py like we do in contrib/asmap/asmap.py.

    The first commit can record the current IndexError with a TODO explaining that it should be skipped. The fix commit then replaces only that assertion.

    I pushed this alternative as two commits to https://github.com/l0rinc/bitcoin/pull/290 for reference. The test can be run with:

    python3 -m unittest contrib.seeds.makeseeds
    
  5. test: characterize makeseeds.py field count check
    Record that a valid 12-field seeder line parses, that shorter lines are
    skipped, and that a line missing only the user agent still aborts the
    run.
    0d0b2cbd97
  6. makeseeds: fix off-by-one in field count check
    DNS seeder lines contain 12 whitespace-separated fields ending with the
    user agent. `parseline()` skipped only lines with fewer than 11 fields
    but reads the user agent from `sline[11]`.
    
    A truncated line with exactly 11 fields therefore raised `IndexError`
    and aborted the run after the asmap database was loaded. Require all 12
    fields so truncated lines are skipped.
    da29409f4b
  7. aman21-droid force-pushed on Sep 2, 2026
  8. aman21-droid commented at 7:15 PM on September 2, 2026: none

    thanks for the reference,i have pushed it accordingly.

  9. l0rinc commented at 7:58 PM on September 2, 2026: contributor

    ACK da29409f4b2e9197e6cc0cfe5a149b403dd27a95

    The PR description should be updated since the tests are in the first commit now and pass on both commits with different expectations. The test run command should also be changed.

  10. aman21-droid commented at 3:43 AM on September 3, 2026: none

    I updated the description and title aswell.

  11. in contrib/seeds/makeseeds.py:288 in 0d0b2cbd97
     283 | +    def test_truncated_line_is_skipped(self):
     284 | +        fields = self.VALID_LINE.split()
     285 | +        for count in range(11):
     286 | +            with self.subTest(fields=count):
     287 | +                self.assertIsNone(parseline(' '.join(fields[:count])))
     288 | +        # TODO: the user agent is the 12th field, so a line with 11 fields should be skipped rather than aborting the run
    


    sedited commented at 2:17 PM on September 7, 2026:

    Seems a bit much to introduce this TODO for a niche utility. Either this should be implemented, or droppped. I think it can be dropped, doesn't seem important to cover this.


    l0rinc commented at 4:25 PM on September 7, 2026:

    The TODO is only in the characterization test so that reveiwers are aware that it's not the desired behavior but the actual one - it're adjusted in the next commit, see https://github.com/bitcoin/bitcoin/blob/db74d3390a391a2a76d7b4d676342a9d1489059b/doc/developer-notes.md?plain=1#L698-L703


    sedited commented at 5:57 PM on September 7, 2026:

    Not sure how I missed that, but yes, of course that is fine!

  12. aman21-droid renamed this:
    contrib: fix off-by-one in makeseeds.py line length check
    makeseeds: fix off-by-one in field count check
    on Sep 7, 2026
  13. achow101 commented at 10:26 PM on September 7, 2026: member

    This file is run by a number of people that is countable on one hand, I don't think a test is particularly useful here, especially as it is not being run in CI, nor should it be.

  14. l0rinc commented at 10:43 PM on September 7, 2026: contributor

    I don't mind if we remove the test now that we have reproduced the problem and the fix, but I suggested that the author adds it similarly to contrib/asmap/asmap.py - the tests are simple, document the usage, I'd keep them

  15. aman21-droid commented at 4:58 AM on September 8, 2026: none

    Apart from the test part this is just a single line fix and I will be happy to remove the test if thats preferred, since it already proved its intended point.


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: 2026-09-09 07:56 UTC