💬 jonatack commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#discussion_r1170242796)
Nice cleanup.
(https://github.com/bitcoin/bitcoin/pull/25390#discussion_r1170242796)
Nice cleanup.
💬 jonatack commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#discussion_r1170250884)
373df61bc56 Style nit, unsure, would something like `local_instance` or `local_name` read better than `name` if callers use named args? Feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/25390#discussion_r1170250884)
373df61bc56 Style nit, unsure, would something like `local_instance` or `local_name` read better than `name` if callers use named args? Feel free to ignore.
💬 kristapsk commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1513473340)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1513473340)
Concept ACK
💬 MarcoFalke commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1513477050)
Unrelated: Looks like there are a few bugs with GCC libstdc++-9:
* https://github.com/bitcoin/bitcoin/issues/27487#issuecomment-1513353311
* clang-8: `./util/bitdeque.h:240:5: error: exception specification of explicitly defaulted move constructor does not match the calculated one`
So I worked around them.
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1513477050)
Unrelated: Looks like there are a few bugs with GCC libstdc++-9:
* https://github.com/bitcoin/bitcoin/issues/27487#issuecomment-1513353311
* clang-8: `./util/bitdeque.h:240:5: error: exception specification of explicitly defaulted move constructor does not match the calculated one`
So I worked around them.
💬 achow101 commented on pull request "[24.1] Bump version to 24.1rc2":
(https://github.com/bitcoin/bitcoin/pull/27486#issuecomment-1513478139)
ACK 03a16a1da09f45721c799c719e48350dfe12ae88
(https://github.com/bitcoin/bitcoin/pull/27486#issuecomment-1513478139)
ACK 03a16a1da09f45721c799c719e48350dfe12ae88
💬 josibake commented on pull request "kernel: chainparams updates for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27482#issuecomment-1513518396)
ACK https://github.com/bitcoin/bitcoin/pull/27482/commits/a2bef805c11a4ab391f4ea635bfde7dd2ec9ce81
verified that I got all the same values on my node for mainnet
(https://github.com/bitcoin/bitcoin/pull/27482#issuecomment-1513518396)
ACK https://github.com/bitcoin/bitcoin/pull/27482/commits/a2bef805c11a4ab391f4ea635bfde7dd2ec9ce81
verified that I got all the same values on my node for mainnet
📝 jonatack opened a pull request: "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x"
(https://github.com/bitcoin/bitcoin/pull/27488)
Update the hardcoded P2P network seeds for 25.x after updating the manual seeds (we needed more Tor and CJDNS seeds, in particular) and the generation script as necessary. Previous update was #25853.
Can be tested by following the steps in `contrib/seeds/README.md`.
Tool output:
```
$ 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…Done.
IPv4 IPv6 Onion Pass
...
(https://github.com/bitcoin/bitcoin/pull/27488)
Update the hardcoded P2P network seeds for 25.x after updating the manual seeds (we needed more Tor and CJDNS seeds, in particular) and the generation script as necessary. Previous update was #25853.
Can be tested by following the steps in `contrib/seeds/README.md`.
Tool output:
```
$ 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…Done.
IPv4 IPv6 Onion Pass
...
💬 jonatack commented on pull request "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1513531812)
Briefly in draft while I run a script to update more manual seeds for reachability/uptime/service bit 1.
(https://github.com/bitcoin/bitcoin/pull/27488#issuecomment-1513531812)
Briefly in draft while I run a script to update more manual seeds for reachability/uptime/service bit 1.
🤔 pablomartin4btc reviewed a pull request: "wallet: improve IBD sync time by skipping block scanning prior birth time"
(https://github.com/bitcoin/bitcoin/pull/27469#pullrequestreview-1390605733)
Concept ACK. Light code review, I'd to spend more time with it, which I'll do soon. Good stuff!
(https://github.com/bitcoin/bitcoin/pull/27469#pullrequestreview-1390605733)
Concept ACK. Light code review, I'd to spend more time with it, which I'll do soon. Good stuff!
💬 fanquake commented on pull request "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170366895)
Manually bumping dates in copyright headers is never required
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170366895)
Manually bumping dates in copyright headers is never required
💬 TheCharlatan commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1513555787)
Updated de2546e672d0a2103228d777b35bfa6aab4881ee -> ec51c252de42e11906f114ac05e476fd88ca2176 ([splitSystemArgs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_2) -> [splitSystemArgs_3](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemArgs_2..splitSystemArgs_3)):
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1374037220) by adding a config
...
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1513555787)
Updated de2546e672d0a2103228d777b35bfa6aab4881ee -> ec51c252de42e11906f114ac05e476fd88ca2176 ([splitSystemArgs_2](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_2) -> [splitSystemArgs_3](https://github.com/TheCharlatan/bitcoin/commits/splitSystemArgs_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemArgs_2..splitSystemArgs_3)):
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1374037220) by adding a config
...
💬 jonatack commented on pull request "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170370860)
Will drop in next push, thanks.
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170370860)
Will drop in next push, thanks.
💬 fanquake commented on pull request "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170376722)
I think this can also just be left as-is. "After removing duplicates" certainly fits the tone/amongst the other sentences better. Not really sure why it needs changing.
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170376722)
I think this can also just be left as-is. "After removing duplicates" certainly fits the tone/amongst the other sentences better. Not really sure why it needs changing.
💬 jonatack commented on pull request "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170387609)
Every other sentence takes the form of a verb followed by an action. I don't mind dropping that, but it seems more in line with the other printed lines.
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170387609)
Every other sentence takes the form of a verb followed by an action. I don't mind dropping that, but it seems more in line with the other printed lines.
💬 jonatack commented on pull request "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170390258)
Skip/remove/enforce/require etc.
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170390258)
Skip/remove/enforce/require etc.
💬 fanquake commented on pull request "p2p: update manual tor/i2p/cjdns mainnet seeds for 25.x":
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170392722)
I don't really mind, I just don't see why there's multiple unrelated changes in this PR (manual copyright header bumping, rewriting log output etc), when it could just be a straight-forward seed update.
(https://github.com/bitcoin/bitcoin/pull/27488#discussion_r1170392722)
I don't really mind, I just don't see why there's multiple unrelated changes in this PR (manual copyright header bumping, rewriting log output etc), when it could just be a straight-forward seed update.
👍 ryanofsky approved a pull request: "refactor: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1390630091)
Code review ACK ec51c252de42e11906f114ac05e476fd88ca2176. Hoping this moveonly change could be reviewed and merged quickly since it conflicts with various PR's I have open
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1390630091)
Code review ACK ec51c252de42e11906f114ac05e476fd88ca2176. Hoping this moveonly change could be reviewed and merged quickly since it conflicts with various PR's I have open
💬 ryanofsky commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170374206)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (ec51c252de42e11906f114ac05e476fd88ca2176)
Any reason these header are only included if WIN32 is not defined? I know the ifndef is preexisting, but it seems like it would make more sense to make these includes unconditional.
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170374206)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (ec51c252de42e11906f114ac05e476fd88ca2176)
Any reason these header are only included if WIN32 is not defined? I know the ifndef is preexisting, but it seems like it would make more sense to make these includes unconditional.
💬 ryanofsky commented on pull request "refactor: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170386744)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (ec51c252de42e11906f114ac05e476fd88ca2176)
Seems like extra whitespace this line.
Note for reviewers: This function previously wasn't exposed as part of the header, but was moved here as part of splitting up args and config file. It can go away with a followup change making config parsing functions more independent.
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1170386744)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (ec51c252de42e11906f114ac05e476fd88ca2176)
Seems like extra whitespace this line.
Note for reviewers: This function previously wasn't exposed as part of the header, but was moved here as part of splitting up args and config file. It can go away with a followup change making config parsing functions more independent.
💬 fanquake commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1513602373)
> What are possible drawbacks here?
I don't really understand adding it given [your comment above](https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1494267607):
> I'd avoid it as it will change code paths being parsed now
You suggested avoiding changing the code paths being parsed, and then suggested adding a define that changes the code paths being parsed?
In its current state, this PR is adding a Boost define, because it has a side-effect that basically "tricks" `clang-ti
...
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1513602373)
> What are possible drawbacks here?
I don't really understand adding it given [your comment above](https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1494267607):
> I'd avoid it as it will change code paths being parsed now
You suggested avoiding changing the code paths being parsed, and then suggested adding a define that changes the code paths being parsed?
In its current state, this PR is adding a Boost define, because it has a side-effect that basically "tricks" `clang-ti
...