💬 ryanofsky commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2456883737)
Went ahead and merged this since it has enough ACKs, and it sounds like the concerns I have about drawbacks of this PR don't seem to be shared by the other reviewers. I think it would be nice if fuzzing could be turned on without turning every else off, and this approach will probably make me a little less likely to start writing fuzz tests, and prefer writing unit tests instead for convenience. But status after this PR isn't worse than the status before #31093, and this approach can always be r
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2456883737)
Went ahead and merged this since it has enough ACKs, and it sounds like the concerns I have about drawbacks of this PR don't seem to be shared by the other reviewers. I think it would be nice if fuzzing could be turned on without turning every else off, and this approach will probably make me a little less likely to start writing fuzz tests, and prefer writing unit tests instead for convenience. But status after this PR isn't worse than the status before #31093, and this approach can always be r
...
💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1829166070)
yes, explaining it more would make sense!
but i think it's out of scope for this PR
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1829166070)
yes, explaining it more would make sense!
but i think it's out of scope for this PR
💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1829170503)
right-it can be used for nested structures, but the call site has to do their own iteration and add it up, as this only covers the outermost layer
adding the `std::is_trivial_v` check is an interesting idea but would prevent it from being used there.
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1829170503)
right-it can be used for nested structures, but the call site has to do their own iteration and add it up, as this only covers the outermost layer
adding the `std::is_trivial_v` check is an interesting idea but would prevent it from being used there.
💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1829176216)
i don't think there's anything to do here; the code is agnostic to the size and implementation of the sso area; the comment gives an example for one C++ library, to illustrate the point
adding more specifics adds more information that could go stale 😄
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1829176216)
i don't think there's anything to do here; the code is agnostic to the size and implementation of the sso area; the comment gives an example for one C++ library, to illustrate the point
adding more specifics adds more information that could go stale 😄
💬 fanquake commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1829177093)
Sure, dropped.
(https://github.com/bitcoin/bitcoin/pull/31172#discussion_r1829177093)
Sure, dropped.
💬 vasild commented on issue "Distribute darknet node addresses via DNS seeds using AAAA records":
(https://github.com/bitcoin/bitcoin/issues/31062#issuecomment-2456901896)
I agree with the reasoning above. I am still Concept ACK if we can have the clients get all addresses from the response, without some addresses being omitted. I see no downsides if separate DNS seeds are used for the new encoding, leaving the existent ones as they are currently.
Just to clarify - Tor only nodes do not use DNS seeds and the intention here is to keep it that way. The motivation is to help multi-homed nodes that use clearnet and Tor. They are currently only getting clearnet addr
...
(https://github.com/bitcoin/bitcoin/issues/31062#issuecomment-2456901896)
I agree with the reasoning above. I am still Concept ACK if we can have the clients get all addresses from the response, without some addresses being omitted. I see no downsides if separate DNS seeds are used for the new encoding, leaving the existent ones as they are currently.
Just to clarify - Tor only nodes do not use DNS seeds and the intention here is to keep it that way. The motivation is to help multi-homed nodes that use clearnet and Tor. They are currently only getting clearnet addr
...
💬 fanquake commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456905576)
@davidgumberg @hodlinator Thanks for taking a look. I've adjusted the subsystem change, and made some other small updates. I think we can do this for now, and follow up further with #30210.
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456905576)
@davidgumberg @hodlinator Thanks for taking a look. I've adjusted the subsystem change, and made some other small updates. I think we can do this for now, and follow up further with #30210.
🤔 hebasto reviewed a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415383222)
Appproach ACK 0440c406eedbc16fa1ce6c77a802b7ea60a79057.
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415383222)
Appproach ACK 0440c406eedbc16fa1ce6c77a802b7ea60a79057.
💬 maflcko commented on pull request "ci: Split out native fuzz jobs for macOS and windows":
(https://github.com/bitcoin/bitcoin/pull/31073#issuecomment-2456949207)
This may be re-considered after a rebase. However, I wonder if the bloat can be reduced, assuming GHA has a matrix feature, so that the non-fuzz and fuzz-build can share most of the GHA config and steps?
(https://github.com/bitcoin/bitcoin/pull/31073#issuecomment-2456949207)
This may be re-considered after a rebase. However, I wonder if the bloat can be reduced, assuming GHA has a matrix feature, so that the non-fuzz and fuzz-build can share most of the GHA config and steps?
💬 maflcko commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2456953234)
> this approach will probably make me a little less likely to start writing fuzz tests
I understand your concerns, but did you actually encounter them in reality? Generally, for writing fuzz tests, you'll want to go with a fuzz engine (and ideally sanitizers). Otherwise, generating fuzz inputs and exploring the fuzz target reachable code will be harder. Once you link a fuzz engine, you'll have to go with a separate fuzz build anyway.
My understanding is that the only real-world developer u
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2456953234)
> this approach will probably make me a little less likely to start writing fuzz tests
I understand your concerns, but did you actually encounter them in reality? Generally, for writing fuzz tests, you'll want to go with a fuzz engine (and ideally sanitizers). Otherwise, generating fuzz inputs and exploring the fuzz target reachable code will be harder. Once you link a fuzz engine, you'll have to go with a separate fuzz build anyway.
My understanding is that the only real-world developer u
...
💬 hebasto commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456981291)
My Guix build:
```
aarch64
061fe1fb0c9d3ce9f564c19d1ddc60127a8fb3cae0cb24b4726c27dd41498c04 guix-build-0440c406eedb/output/dist-archive/bitcoin-0440c406eedb.tar.gz
2a0198646e6cbf8c61baf683bf36a25a826d19b8941e7b3a9cfeb04914d39aed guix-build-0440c406eedb/output/x86_64-w64-mingw32/SHA256SUMS.part
bdc9fb82b0f1a5dd361392f41aa09fe158448009f92c9914c04096d9ca47458c guix-build-0440c406eedb/output/x86_64-w64-mingw32/bitcoin-0440c406eedb-win64-debug.zip
896dcec8adae706035a693bf56a8ec8b3a88a5fb44e8
...
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456981291)
My Guix build:
```
aarch64
061fe1fb0c9d3ce9f564c19d1ddc60127a8fb3cae0cb24b4726c27dd41498c04 guix-build-0440c406eedb/output/dist-archive/bitcoin-0440c406eedb.tar.gz
2a0198646e6cbf8c61baf683bf36a25a826d19b8941e7b3a9cfeb04914d39aed guix-build-0440c406eedb/output/x86_64-w64-mingw32/SHA256SUMS.part
bdc9fb82b0f1a5dd361392f41aa09fe158448009f92c9914c04096d9ca47458c guix-build-0440c406eedb/output/x86_64-w64-mingw32/bitcoin-0440c406eedb-win64-debug.zip
896dcec8adae706035a693bf56a8ec8b3a88a5fb44e8
...
💬 vasild commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2456985760)
> We keep the DNS seeds, but treat them always as addrfetch peers
> Another downside would be that finding an initial batch of peers would take longer
What about having P2P-seeds, an alternative to DNS-seeds, which are serving the P2P protocol and are used as addrfetch peers and they return only high-quality addresses from the crawler? The IP/Tor/I2P addresses of those P2P-seeds could be hardcoded or (for clearnet only) we can have hardcoded the hostname, e.g. `p2pseed.bitcoin.org` which r
...
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2456985760)
> We keep the DNS seeds, but treat them always as addrfetch peers
> Another downside would be that finding an initial batch of peers would take longer
What about having P2P-seeds, an alternative to DNS-seeds, which are serving the P2P protocol and are used as addrfetch peers and they return only high-quality addresses from the crawler? The IP/Tor/I2P addresses of those P2P-seeds could be hardcoded or (for clearnet only) we can have hardcoded the hostname, e.g. `p2pseed.bitcoin.org` which r
...
👍 rkrux approved a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2415368101)
reACK c189eec848e3c31f438151d4d3422718a29df3a3
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2415368101)
reACK c189eec848e3c31f438151d4d3422718a29df3a3
💬 rkrux commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829194254)
I ran the node with `mempoolfullrbf=0` and it correctly ignored the option.
```
2024-11-05T09:53:52Z Ignoring unknown configuration value regtest.mempoolfullrbf
```
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829194254)
I ran the node with `mempoolfullrbf=0` and it correctly ignored the option.
```
2024-11-05T09:53:52Z Ignoring unknown configuration value regtest.mempoolfullrbf
```
💬 rkrux commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829209285)
After the PR is merged: Thinking out loud, should the `fullrbf` option in this RPC output still be shown at all? Would it hold any meaningful significance or would it be just unnecessary information?
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829209285)
After the PR is merged: Thinking out loud, should the `fullrbf` option in this RPC output still be shown at all? Would it hold any meaningful significance or would it be just unnecessary information?
👍 hebasto approved a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160)
ACK 0440c406eedbc16fa1ce6c77a802b7ea60a79057, the [Guix-built](https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456981291) `test_bitcoin.exe`, `bitcoind.exe`, `bitcoin-qt.exe`, and the installer have been tested on Windows 11 Pro 23H2.
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160)
ACK 0440c406eedbc16fa1ce6c77a802b7ea60a79057, the [Guix-built](https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2456981291) `test_bitcoin.exe`, `bitcoind.exe`, `bitcoin-qt.exe`, and the installer have been tested on Windows 11 Pro 23H2.
💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2457018427)
Are you still working on it?
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2457018427)
Are you still working on it?
💬 ryanofsky commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457030352)
> I understand your concerns, but did you actually encounter them in reality?
Yes, I've never written or run a fuzz test because they require separate setup and because I'm reasonably happy writing unit tests. If I could run fuzz tests in my default build, I'd be more likely to write fuzz tests as an alternative to unit tests. I understand fuzz tests are not always an alternative to unit tests, but I've written a lot of unit tests where I'm just handcrafting inputs to test edge cases, and the
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457030352)
> I understand your concerns, but did you actually encounter them in reality?
Yes, I've never written or run a fuzz test because they require separate setup and because I'm reasonably happy writing unit tests. If I could run fuzz tests in my default build, I'd be more likely to write fuzz tests as an alternative to unit tests. I understand fuzz tests are not always an alternative to unit tests, but I've written a lot of unit tests where I'm just handcrafting inputs to test edge cases, and the
...
🤔 noelportillo reviewed a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2415492049)
The only way to make the money
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2415492049)
The only way to make the money
💬 sipa commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457048406)
@ryanofsky As long as we're talking about libfuzzer-based fuzzing (which is what we mostly use in this project, though not exclusively), the `-fsanitize=fuzzer` option does two things:
1. It instruments the code to keep track of branch/code coverage in a libfuzzer-compatible way
2. It add the libfuzzer main() function.
These pretty much inherently require a separate build: you can't do fuzzing without (1), because the mutations that libfuzzer makes to the corpus are based on coverage inform
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457048406)
@ryanofsky As long as we're talking about libfuzzer-based fuzzing (which is what we mostly use in this project, though not exclusively), the `-fsanitize=fuzzer` option does two things:
1. It instruments the code to keep track of branch/code coverage in a libfuzzer-compatible way
2. It add the libfuzzer main() function.
These pretty much inherently require a separate build: you can't do fuzzing without (1), because the mutations that libfuzzer makes to the corpus are based on coverage inform
...