📝 theuni opened a pull request: "depends: reuse _config_opts for CMake options"
(https://github.com/bitcoin/bitcoin/pull/27496)
Context: I'm [currently experimenting with building our depends with CMake when possible](https://github.com/theuni/bitcoin/commits/depends-cmake) as part of our future transition to CMake. Specifically zmq, libevent, libnatpmp, and miniupnpc all have existing CMake buildsystems. Building them with CMake will allow us to drop several deps that we currently have (autoconf, automake, pkg-config, etc) which would be unfortunate to carry over after the switch-over.
But that's not relevant for thi
...
(https://github.com/bitcoin/bitcoin/pull/27496)
Context: I'm [currently experimenting with building our depends with CMake when possible](https://github.com/theuni/bitcoin/commits/depends-cmake) as part of our future transition to CMake. Specifically zmq, libevent, libnatpmp, and miniupnpc all have existing CMake buildsystems. Building them with CMake will allow us to drop several deps that we currently have (autoconf, automake, pkg-config, etc) which would be unfortunate to carry over after the switch-over.
But that's not relevant for thi
...
💬 theuni commented on pull request "depends: reuse _config_opts for CMake options":
(https://github.com/bitcoin/bitcoin/pull/27496#issuecomment-1515416038)
Ping @hebasto. This should be easy to review, only verifying that it's currently a no-op :)
(https://github.com/bitcoin/bitcoin/pull/27496#issuecomment-1515416038)
Ping @hebasto. This should be easy to review, only verifying that it's currently a no-op :)
💬 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.