💬 jonatack commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171880911)
It looks like this will never be true, unless a user manually adds `#` characters to the start of lines. It's not hit currently.
```diff
if line.startswith('#'):
# Ignore line that starts with comment
+ print(line.split(), file=sys.stderr)
return None
```
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171880911)
It looks like this will never be true, unless a user manually adds `#` characters to the start of lines. It's not hit currently.
```diff
if line.startswith('#'):
# Ignore line that starts with comment
+ print(line.split(), file=sys.stderr)
return None
```
💬 jonatack commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171884500)
Verified that this fix works with the following diff:
```diff
# Skip bad results.
if int(sline[1]) == 0:
+ # print(line.split(), file=sys.stderr)
return None
```
The result of running the script:
before
```
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds…Done.
IPv4 IPv6 Onion Pass
99253 23129 0 Initial
99253 23129 0 Skip entries with invalid address
99
...
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171884500)
Verified that this fix works with the following diff:
```diff
# Skip bad results.
if int(sline[1]) == 0:
+ # print(line.split(), file=sys.stderr)
return None
```
The result of running the script:
before
```
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds…Done.
IPv4 IPv6 Onion Pass
99253 23129 0 Initial
99253 23129 0 Skip entries with invalid address
99
...
💬 fanquake commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171884989)
> It looks like this will never be true, unless a user manually adds # characters
The first line seeds_main.txt is # addresses, so it's going to be hit at least once.
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171884989)
> It looks like this will never be true, unless a user manually adds # characters
The first line seeds_main.txt is # addresses, so it's going to be hit at least once.
👍 jonatack approved a pull request: "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`"
(https://github.com/bitcoin/bitcoin/pull/26681#pullrequestreview-1392930267)
ACK 3cc989da5c750e740705131bed05bbf93bfdf169
(https://github.com/bitcoin/bitcoin/pull/26681#pullrequestreview-1392930267)
ACK 3cc989da5c750e740705131bed05bbf93bfdf169
💬 achow101 commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171886146)
It catches the first line for me.
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171886146)
It catches the first line for me.
💬 jonatack commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171886252)
Yes. Updated my comment to mention the first line and result.
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171886252)
Yes. Updated my comment to mention the first line and result.
💬 jonatack commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171889143)
Actually, it may make more sense to leave the '#' conditional where it is, as it is only hit once, and instead do:
```diff
# Skip bad results.
- if int(sline[1]) == 0:
+ if sline[1] == '0':
return None
```
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171889143)
Actually, it may make more sense to leave the '#' conditional where it is, as it is only hit once, and instead do:
```diff
# Skip bad results.
- if int(sline[1]) == 0:
+ if sline[1] == '0':
return None
```
💬 hebasto commented on pull request "depends: reuse _config_opts for CMake options":
(https://github.com/bitcoin/bitcoin/pull/27496#issuecomment-1515432548)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27496#issuecomment-1515432548)
Concept ACK.
🤔 mzumsande reviewed a pull request: "p2p: update hardcoded mainnet seeds for 25.x"
(https://github.com/bitcoin/bitcoin/pull/27488#pullrequestreview-1392969258)
ACK b330a6621d8801ec4c408dc0ca69ec2d02b88564
Not 100% sure how to best test this, but I did the following:
I followed the steps described in `contrib/seeds/README.md`, the overall result is similar.
There are some differences (~60 in total) - I guess that's expected due to changed input data and asmap data from sipa's seeder.
```
$ python3 makeseeds.py -a asmap-filled.dat -s seeds_main.txt > nodes_main.txt
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds
...
(https://github.com/bitcoin/bitcoin/pull/27488#pullrequestreview-1392969258)
ACK b330a6621d8801ec4c408dc0ca69ec2d02b88564
Not 100% sure how to best test this, but I did the following:
I followed the steps described in `contrib/seeds/README.md`, the overall result is similar.
There are some differences (~60 in total) - I guess that's expected due to changed input data and asmap data from sipa's seeder.
```
$ python3 makeseeds.py -a asmap-filled.dat -s seeds_main.txt > nodes_main.txt
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds
...
👍 hebasto approved a pull request: "depends: reuse _config_opts for CMake options"
(https://github.com/bitcoin/bitcoin/pull/27496#pullrequestreview-1392993005)
ACK 63c0c4ff10b2f6ed8510ef372a5b1f6a6494b179
This change allows to reuse this machinery: https://github.com/bitcoin/bitcoin/blob/d26a71a94ac4ae1b1a091f4412d390afba69b2f8/depends/funcs.mk#L128-L131
(https://github.com/bitcoin/bitcoin/pull/27496#pullrequestreview-1392993005)
ACK 63c0c4ff10b2f6ed8510ef372a5b1f6a6494b179
This change allows to reuse this machinery: https://github.com/bitcoin/bitcoin/blob/d26a71a94ac4ae1b1a091f4412d390afba69b2f8/depends/funcs.mk#L128-L131
💬 jonatack commented on pull request "p2p: update hardcoded mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1515510969)
> I think it's great that we now have a few more tor seeds than 11, which seemed rather low to me considering the large number of bitcoin nodes reachable via tor.
Credit to you, @mzumsande, for suggesting to me yesterday that we add more.
I've repushed after pulling in 3cc989da5c750e740705131bed05bbf93bfdf169 from #26681 and re-running the steps in `contrib/seeds/README.md`.
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1515510969)
> I think it's great that we now have a few more tor seeds than 11, which seemed rather low to me considering the large number of bitcoin nodes reachable via tor.
Credit to you, @mzumsande, for suggesting to me yesterday that we add more.
I've repushed after pulling in 3cc989da5c750e740705131bed05bbf93bfdf169 from #26681 and re-running the steps in `contrib/seeds/README.md`.
💬 yusufsahinhamza commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171947351)
Yes, i've meant to ignore the first line (the comment for column names) in `seeds_main.txt` by putting the **"#"** conditional.
@jonatack I've thought about comparing it in **str** but then decided to cast with **int** because, seems like the `good` column meant to be **0 and 1** which is **integer**, so i rather use casting and have the **"#"** conditional.
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171947351)
Yes, i've meant to ignore the first line (the comment for column names) in `seeds_main.txt` by putting the **"#"** conditional.
@jonatack I've thought about comparing it in **str** but then decided to cast with **int** because, seems like the `good` column meant to be **0 and 1** which is **integer**, so i rather use casting and have the **"#"** conditional.
💬 jonatack commented on pull request "net, contrib: Bugfix for checking bad dns seeds without casting in `makeseeds.py`":
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171950558)
@yusufsahinhamza I cherry-picked your commit here as-is into #27488 to start using it for generating the hardcoded seeds in the v25 release.
(https://github.com/bitcoin/bitcoin/pull/26681#discussion_r1171950558)
@yusufsahinhamza I cherry-picked your commit here as-is into #27488 to start using it for generating the hardcoded seeds in the v25 release.
💬 theStack commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1171962843)
tiny nit: could be put in the "private:" section, since there is a static constexpr `size()` function returning this value below anyway.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1171962843)
tiny nit: could be put in the "private:" section, since there is a static constexpr `size()` function returning this value below anyway.
💬 theStack commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1171972333)
There's a theoretical possibility that this call crashes due to failed assertion `assert(fValid)`, if the generated key from the previous round (=first 32 bytes of ellswift encoded pubkey) was not valid. Due to the practical low probably for this to ever happen (if my math isn't off, on average for every 2^(127,65) randomly generated key is invalid), I guess it's not worth it to add any checks here, especially since it's only a benchmark?
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1171972333)
There's a theoretical possibility that this call crashes due to failed assertion `assert(fValid)`, if the generated key from the previous round (=first 32 bytes of ellswift encoded pubkey) was not valid. Due to the practical low probably for this to ever happen (if my math isn't off, on average for every 2^(127,65) randomly generated key is invalid), I guess it's not worth it to add any checks here, especially since it's only a benchmark?
💬 jonatack commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1515600786)
Again here https://cirrus-ci.com/task/5250935128064000?logs=ci#L3275
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1515600786)
Again here https://cirrus-ci.com/task/5250935128064000?logs=ci#L3275
⚠️ ariard opened an issue: "Document CoreDev organization"
(https://github.com/bitcoin/bitcoin/issues/27497)
### Please describe the feature you'd like to see added.
They have been attempts and discussions in the past to document other aspects of project culture such as maintenance with #25560. The organization of in-person events can be itself a source of contention, and I'm speaking here as a former
organizer. While it's voluntary like all other tasks in Bitcoin open-source, there is a lot of subjectivity in the choice of schedule, place and the selection of attendees. Documenting such process will
...
(https://github.com/bitcoin/bitcoin/issues/27497)
### Please describe the feature you'd like to see added.
They have been attempts and discussions in the past to document other aspects of project culture such as maintenance with #25560. The organization of in-person events can be itself a source of contention, and I'm speaking here as a former
organizer. While it's voluntary like all other tasks in Bitcoin open-source, there is a lot of subjectivity in the choice of schedule, place and the selection of attendees. Documenting such process will
...
💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1172071849)
I think this should be:
```c++
if (!inner.m_opts.also_positional) {
CHECK_NONFATAL(named_args.insert(inner_name).second);
}
```
Otherwise a duplicate positional parameter after the `OBJ_NAMED_PARAMS` entry will give an error.
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1172071849)
I think this should be:
```c++
if (!inner.m_opts.also_positional) {
CHECK_NONFATAL(named_args.insert(inner_name).second);
}
```
Otherwise a duplicate positional parameter after the `OBJ_NAMED_PARAMS` entry will give an error.
🤔 ajtowns reviewed a pull request: "refactor: Move chain constants to the util library"
(https://github.com/bitcoin/bitcoin/pull/27491#pullrequestreview-1393199662)
Would be better if the second commit (renaming `CBaseChainParam::MAIN` to `ChainType::MAIN` etc) could be a scripted-diff. Perhaps structure it as:
* introduce ChainType
* add `CreateBaseChainParams(ChainType)` and replace `CreateBaseChainParams(string)` with `{ auto ct = ChainTypeFromString(chain); if (ct) { return CreateBaseChainParams(ct); } else { throw...; }` (and so on)
* scripted-diff to switch to `ChainType::MAIN` and `args.GetChainType()`
* remove the now unused functions that
...
(https://github.com/bitcoin/bitcoin/pull/27491#pullrequestreview-1393199662)
Would be better if the second commit (renaming `CBaseChainParam::MAIN` to `ChainType::MAIN` etc) could be a scripted-diff. Perhaps structure it as:
* introduce ChainType
* add `CreateBaseChainParams(ChainType)` and replace `CreateBaseChainParams(string)` with `{ auto ct = ChainTypeFromString(chain); if (ct) { return CreateBaseChainParams(ct); } else { throw...; }` (and so on)
* scripted-diff to switch to `ChainType::MAIN` and `args.GetChainType()`
* remove the now unused functions that
...
💬 ajtowns commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1172082054)
Calling it `ChainName(ChainType chain)` might be a bit clearer?
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1172082054)
Calling it `ChainName(ChainType chain)` might be a bit clearer?
💬 ajtowns commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1172076982)
Put `MAIN` on a newline and just use 4-space indentation? Newlines for `{` and `}` as well, per style guide? Trailing `,` after `REGTEST` in case we add any new ones later?
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1172076982)
Put `MAIN` on a newline and just use 4-space indentation? Newlines for `{` and `}` as well, per style guide? Trailing `,` after `REGTEST` in case we add any new ones later?