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

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.

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	maflcko, 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. aman21-droid force-pushed on Sep 2, 2026
6. 
   aman21-droid commented at 7:15 PM on September 2, 2026: none

   thanks for the reference,i have pushed it accordingly.

7. 
   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.

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

   I updated the description and title aswell.

9. 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!

10. 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
11. 
    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.

12. 
    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

13. 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.

    c786052865
14. 
    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.

15. 
    maflcko commented at 5:55 PM on September 17, 2026: member

    I think it is fine to copy-paste the test into a comment here. Anyone who wants to read or run it, can do so. But there won't be any dead code added to maintain in the future.

16. 
    maflcko commented at 10:25 AM on September 24, 2026: member

    Are you still working on this, or can it be closed?

17. aman21-droid force-pushed on Sep 30, 2026
18. 
    aman21-droid commented at 6:37 AM on September 30, 2026: none

    Removed the test, so this is now just the one line fix. Pasting the test here as suggested, so anyone who wants to read or run it can:

    class TestParseLine(unittest.TestCase):
        """Unit tests for `parseline`."""
    
        # A seeder line has 12 whitespace-separated fields: address, good, lastSuccess, %(2h), %(8h), %(1d), %(7d), %(30d), blocks, services, version, user agent
        VALID_LINE = '1.2.3.4:8333 1 1700000000 100.00% 100.00% 100.00% 100.00% 100.00% 910001 000000000000040d 70016 "/Satoshi:29.0.0/"'
    
        def test_valid_line(self):
            parsed = parseline(self.VALID_LINE)
            self.assertEqual(parsed['net'], 'ipv4')
            self.assertEqual(parsed['ip'], '1.2.3.4')
            self.assertEqual(parsed['port'], 8333)
            self.assertEqual(parsed['agent'], '/Satoshi:29.0.0/')
    
        def test_truncated_line_is_skipped(self):
            fields = self.VALID_LINE.split()
            for count in range(11):
                with self.subTest(fields=count):
                    self.assertIsNone(parseline(' '.join(fields[:count])))
            self.assertIsNone(parseline(' '.join(fields[:11])))  # The user agent is the 12th field, so 11 fields is still too short
    

    It goes in makeseeds.py next to import unittest, and runs with python3 -m unittest contrib.seeds.makeseeds.

19. 
    maflcko commented at 6:48 AM on September 30, 2026: member

    lgtm ACK c786052865fd173fd8bc7396e1303f7840c5b0da

    Seems fine to change this, and probably doesn't matter because a corrupt file that has exactly 11 fields in one line (instead of less than 11) seems impossibly rare. Also, the real problem here would be the corrupt file, not the fact that this script crashes.

20. DrahtBot requested review from l0rinc on Sep 30, 2026
21. 
    l0rinc commented at 5:25 PM on September 30, 2026: contributor

    ACK c786052865fd173fd8bc7396e1303f7840c5b0da

Contributors

aman21-droidDrahtBotl0rincseditedachow101maflcko

Review Requested
l0rinc