💬 maflcko commented on pull request "fuzz: add more coverage for `ScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/30134#discussion_r1605702378)
Can you explain this change?
```
wallet/test/fuzz/scriptpubkeyman.cpp should add these lines:
#include <assert.h> // for assert
#include <functional> // for function
#include <map> // for map
#include <memory> // for unique_ptr, shared_ptr, make_unique, __shared_ptr_access
#include <optional> // for optional, nullopt, nullopt_t
#include <unordered_set>
...
(https://github.com/bitcoin/bitcoin/pull/30134#discussion_r1605702378)
Can you explain this change?
```
wallet/test/fuzz/scriptpubkeyman.cpp should add these lines:
#include <assert.h> // for assert
#include <functional> // for function
#include <map> // for map
#include <memory> // for unique_ptr, shared_ptr, make_unique, __shared_ptr_access
#include <optional> // for optional, nullopt, nullopt_t
#include <unordered_set>
...
💬 maflcko commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118697981)
utACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118697981)
utACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
💬 maflcko commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2118706781)
Not sure what's wrong with the CI ( https://github.com/bitcoin/bitcoin/actions/runs/9133680294/job/25117694444?pr=30132#step:6:2 ) Maybe rebase again?
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2118706781)
Not sure what's wrong with the CI ( https://github.com/bitcoin/bitcoin/actions/runs/9133680294/job/25117694444?pr=30132#step:6:2 ) Maybe rebase again?
💬 brunoerg commented on pull request "fuzz: add more coverage for `ScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/30134#discussion_r1605721747)
Nevermind, just dropped this change.
(https://github.com/bitcoin/bitcoin/pull/30134#discussion_r1605721747)
Nevermind, just dropped this change.
💬 rkrux commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605724296)
Hmm I see now what you mean.
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605724296)
Hmm I see now what you mean.
💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605724571)
Sure, but then the test will fail regardless. Might as well fail early, if the peer disconnects again, no?
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605724571)
Sure, but then the test will fail regardless. Might as well fail early, if the peer disconnects again, no?
💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2118731144)
re-ACK 09ef322acc0a88a9e119f74923399598984c68f6 . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2118731144)
re-ACK 09ef322acc0a88a9e119f74923399598984c68f6 . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.
👍 rkrux approved a pull request: "test: Remove struct.pack from almost all places"
(https://github.com/bitcoin/bitcoin/pull/29401#pullrequestreview-2064681964)
tACK [fa52e13](https://github.com/bitcoin/bitcoin/pull/29401/commits/fa52e13ee81fcc7543890dbd6986fcb55168583f)
Make successful, so are all the functional tests.
Overall I agree with the intent to get rid of `struct.pack` usage, which requires looking up the documentation most of the time and might be prone to errors. Using the alternate approach of `to_bytes` is more self-documenting and preferable to me.
(https://github.com/bitcoin/bitcoin/pull/29401#pullrequestreview-2064681964)
tACK [fa52e13](https://github.com/bitcoin/bitcoin/pull/29401/commits/fa52e13ee81fcc7543890dbd6986fcb55168583f)
Make successful, so are all the functional tests.
Overall I agree with the intent to get rid of `struct.pack` usage, which requires looking up the documentation most of the time and might be prone to errors. Using the alternate approach of `to_bytes` is more self-documenting and preferable to me.
📝 hebasto opened a pull request: "build: Disable `_FORTIFY_SOURCE` if using MSan"
(https://github.com/bitcoin/bitcoin/pull/30140)
This PR shifts responsibility to disable `_FORTIFY_SOURCE` from the user to the build system, which:
- improves usability
- simplifies the CI and OSS-Fuzz scripts
- is useful for CMake migration as it removes the need to use non-standard `APPEND_CPPFLAGS` in such a corner case
(https://github.com/bitcoin/bitcoin/pull/30140)
This PR shifts responsibility to disable `_FORTIFY_SOURCE` from the user to the build system, which:
- improves usability
- simplifies the CI and OSS-Fuzz scripts
- is useful for CMake migration as it removes the need to use non-standard `APPEND_CPPFLAGS` in such a corner case
💬 fanquake commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118764778)
Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it's possible to override them using standard CMake variables. Not push more complexity into the build system, and hardcode specific things.
Removing APPEND_*FLAGS shouldn't be a goal, as it needs to exist, to do what it exists for (actually give a mechanism for always overriding a flag, regardless of what CMake try's to do). Otherwise in future, if we (or any oth
...
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118764778)
Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it's possible to override them using standard CMake variables. Not push more complexity into the build system, and hardcode specific things.
Removing APPEND_*FLAGS shouldn't be a goal, as it needs to exist, to do what it exists for (actually give a mechanism for always overriding a flag, regardless of what CMake try's to do). Otherwise in future, if we (or any oth
...
💬 hebasto commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118770672)
> Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it's possible to override them using standard CMake variables.
I disagree. The current implementation is undocumented and error-prone on the user side.
> Not push more complexity into the build system, and hardcode specific things.
This statement seems quite arbitrary. Considering that the build system is full of "hardcode specific things" that "push mor
...
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118770672)
> Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it's possible to override them using standard CMake variables.
I disagree. The current implementation is undocumented and error-prone on the user side.
> Not push more complexity into the build system, and hardcode specific things.
This statement seems quite arbitrary. Considering that the build system is full of "hardcode specific things" that "push mor
...
💬 fanquake commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118773169)
> I disagree. The current implementation is undocumented and error-prone on the user side.
In the CMake branch? If this is the case, then it needs to be reworked anyway...?
Another reason undefining a flag like this needs to be easily possible, is if you want to set the fortify level to anything other than what we pick, it needs to be undefined first. So this all needs to be possible outside of MSAN. Given that, we should also use the same mechanisms, rather than hardcoding things here.
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118773169)
> I disagree. The current implementation is undocumented and error-prone on the user side.
In the CMake branch? If this is the case, then it needs to be reworked anyway...?
Another reason undefining a flag like this needs to be easily possible, is if you want to set the fortify level to anything other than what we pick, it needs to be undefined first. So this all needs to be possible outside of MSAN. Given that, we should also use the same mechanisms, rather than hardcoding things here.
💬 hebasto commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118778427)
> > I disagree. The current implementation is undocumented and error-prone on the user side.
>
> In the CMake branch? If this is the case, then it needs to be reworked anyway...?
In the master branch.
`./configure CC=clang CXX=clang++ --with-sanitizers=memory` just leads to MSan warnings forcing the user for further actions.
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118778427)
> > I disagree. The current implementation is undocumented and error-prone on the user side.
>
> In the CMake branch? If this is the case, then it needs to be reworked anyway...?
In the master branch.
`./configure CC=clang CXX=clang++ --with-sanitizers=memory` just leads to MSan warnings forcing the user for further actions.
💬 fanquake commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118782301)
> ./configure CC=clang CXX=clang++ --with-sanitizers=memory just leads to MSan warnings forcing the user for further actions.
Your PR here doesn't change that. Anyone wanting to use MSAN needs to do extensive setup, and build a fully instrumented toolchain. It can't be expected to work out of the box.
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118782301)
> ./configure CC=clang CXX=clang++ --with-sanitizers=memory just leads to MSan warnings forcing the user for further actions.
Your PR here doesn't change that. Anyone wanting to use MSAN needs to do extensive setup, and build a fully instrumented toolchain. It can't be expected to work out of the box.
💬 hebasto commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118792426)
> Anyone wanting to use MSAN needs to do extensive setup, and build a fully instrumented toolchain. It can't be expected to work out of the box.
Right. I didn't mean the latter. Just quoted the main system configuration stage for brevity.
> Your PR here doesn't change that.
It does. The second commit proves it, doesn't it?
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118792426)
> Anyone wanting to use MSAN needs to do extensive setup, and build a fully instrumented toolchain. It can't be expected to work out of the box.
Right. I didn't mean the latter. Just quoted the main system configuration stage for brevity.
> Your PR here doesn't change that.
It does. The second commit proves it, doesn't it?
💬 fanquake commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118793451)
> It does. The second commit proves it, doesn't it?
No. I don't see how that commit sets up a fully instrumented MSAN toolchain.
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118793451)
> It does. The second commit proves it, doesn't it?
No. I don't see how that commit sets up a fully instrumented MSAN toolchain.
💬 hebasto commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118794938)
> > It does. The second commit proves it, doesn't it?
>
> No. I don't see how that commit sets up a fully instrumented MSAN toolchain.
Sure. It "shifts responsibility to disable `_FORTIFY_SOURCE` from the user to the build system".
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118794938)
> > It does. The second commit proves it, doesn't it?
>
> No. I don't see how that commit sets up a fully instrumented MSAN toolchain.
Sure. It "shifts responsibility to disable `_FORTIFY_SOURCE` from the user to the build system".
💬 fanquake commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118796310)
> Sure. It "shifts responsibility to disable _FORTIFY_SOURCE from the user to the build system".
Setting a single preprocessor flag isn't instrumenting a toolchain?
I still haven't seen a good reason to not use the mechanisms that exist to set flags, given those mechanisms need to exist (i.e undefining and redefining fortify source), and should be tested so they are known to be working.
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118796310)
> Sure. It "shifts responsibility to disable _FORTIFY_SOURCE from the user to the build system".
Setting a single preprocessor flag isn't instrumenting a toolchain?
I still haven't seen a good reason to not use the mechanisms that exist to set flags, given those mechanisms need to exist (i.e undefining and redefining fortify source), and should be tested so they are known to be working.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2118804764)
MacOS implementation of default gateway finding was added as well (i've tested it on MacOS Monterey, but could always use more). This concludes the coverage of default gateway-finding on the major platforms.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2118804764)
MacOS implementation of default gateway finding was added as well (i've tested it on MacOS Monterey, but could always use more). This concludes the coverage of default gateway-finding on the major platforms.
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605795010)
ah ok, yeah. Now we are sync. Pushed. Thanks.
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605795010)
ah ok, yeah. Now we are sync. Pushed. Thanks.