💬 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
...
💬 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-2457071014)
> Yes, I've never written or run a fuzz test because they require separate setup
I see. I think what you are asking for could be achieved by reverting this pull request, possibly hiding the performance impact behind `-DCMAKE_BUILD_TYPE=Debug` (or another cmake dev-only flag). Then, you could use the fuzz executable from the "normal" build (without sanitizers and a fuzz engine) for blackbox fuzzing in AFL++ QEMU mode (or something similar). However, for me personally just going with a separate
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457071014)
> Yes, I've never written or run a fuzz test because they require separate setup
I see. I think what you are asking for could be achieved by reverting this pull request, possibly hiding the performance impact behind `-DCMAKE_BUILD_TYPE=Debug` (or another cmake dev-only flag). Then, you could use the fuzz executable from the "normal" build (without sanitizers and a fuzz engine) for blackbox fuzzing in AFL++ QEMU mode (or something similar). However, for me personally just going with a separate
...
💬 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-2457117169)
Thanks for clarifications and ideas. Iit sounds like doing what I want would require `fuzzer-no-link` instead of `fuzzer` but otherwise it should be possible for BUILD_FOR_FUZZING to just enable extra instrumentation and not make every library and every other binary unusable. To me this just seems like a most straightforward way and least surprising way of organizing the build, that would make fuzzing easier to get started with by just being able to turn on an option that doesn't break everythin
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2457117169)
Thanks for clarifications and ideas. Iit sounds like doing what I want would require `fuzzer-no-link` instead of `fuzzer` but otherwise it should be possible for BUILD_FOR_FUZZING to just enable extra instrumentation and not make every library and every other binary unusable. To me this just seems like a most straightforward way and least surprising way of organizing the build, that would make fuzzing easier to get started with by just being able to turn on an option that doesn't break everythin
...