Skip to content

contrib: improve makeseeds.py progress indicator #24863

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.

Sign up for GitHub

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

Jump to bottom
Closed
jonatack wants to merge 1 commit into from
Closed

contrib: improve makeseeds.py progress indicator #24863

jonatack wants to merge 1 commit into from

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Apr 15, 2022 •
edited
Loading

Follow-up to #24818.

before

  IPv4   IPv6  Onion Pass
470813  73279      0 Initial
470813  73279      0 Skip entries with invalid address
470813  73279      0 After removing duplicates
470812  73279      0 Skip entries from suspicious hosts
  6340   1733      0 Enforce minimal number of blocks
  5480   1490      0 Require service bit 1
  3892    880      0 Require minimum uptime
  3818    856      0 Require a known and recent user agent
  3789    848      0 Filter out hosts with multiple bitcoin ports
   160 / 4637 [03.5%]       <- user wonders what is happening now and a leading zero is printed
   512    512      0 Look up ASNs and limit results per ASN and per net

after

470813  73279      0 Initial
470813  73279      0 Skip entries with invalid address
470813  73279      0 After removing duplicates
470812  73279      0 Skip entries from suspicious hosts
  6340   1733      0 Enforce minimal number of blocks
  5480   1490      0 Require service bit 1
  3892    880      0 Require minimum uptime
  3818    856      0 Require a known and recent user agent
  3789    848      0 Filter out hosts with multiple bitcoin ports
                     Look up ASNs and limit results per ASN and per net:   160 / 4637 (3.5%)
   512    512      0 Look up ASNs and limit results per ASN and per net  <- replaces previous line when done

@jonatack jonatack force-pushed the improve-makeseeds-progress-indicator branch from ad81777 to 9bda597 Compare April 15, 2022 14:34
@jonatack jonatack force-pushed the improve-makeseeds-progress-indicator branch from 9bda597 to e937121 Compare April 15, 2022 14:57
@laanwj
Copy link
Member

laanwj commented Apr 15, 2022

Concept ACK. Though not sure we still need a progress indicator after #24864, lookup is really really fast with asmap.

@jonatack
Copy link
Member Author

jonatack commented Apr 15, 2022

Even better then, will test.

vincenzopalazzo
vincenzopalazzo reviewed Apr 15, 2022
View reviewed changes
Copy link

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Apr 16, 2022
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 16, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24864 (contrib: Use asmap for ASN lookup in makeseeds by laanwj)

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.

@jonatack
Copy link
Member Author

jonatack commented Apr 18, 2022

Concept ACK. Though not sure we still need a progress indicator after #24864, lookup is really really fast with asmap.

After testing it, yes it might be better to remove the indicator there and maybe add one when loading the seed data.

@fanquake
Copy link
Member

fanquake commented Apr 18, 2022

Great. Lets close this for now then, and keep the commentary / removal / figuring out if we even need a progress indicator in #24864.

@fanquake fanquake closed this Apr 18, 2022
@jonatack
Copy link
Member Author

jonatack commented Apr 18, 2022

That's not really what that pull is currently about, though, so I thought this one could be updated after. Oh well.

@fanquake
Copy link
Member

fanquake commented Apr 18, 2022

That's not really what that pull is currently about, though, so I thought this one could be updated after. Oh well.

The pull doesn't only have to be about the progress indicator, to make decisions about it's existence. Given the changes, one of the side-effects may be that we just no longer need the progress indicator, so it probably make sense for that discussion/change to happen in that PR. Rather than following up again afterwards.

@jonatack
Copy link
Member Author

jonatack commented Apr 18, 2022

Maybe let that process happen naturally?

@bitcoin bitcoin locked and limited conversation to collaborators Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@vincenzopalazzo vincenzopalazzo vincenzopalazzo left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Scripts and tools

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jonatack @laanwj @DrahtBot @fanquake @vincenzopalazzo