💬 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.
💬 marcofleon commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2118863790)
If I don't start with a seed corpus (inputs from scratch) then it seems to run fine. I let it go for a while and no crashes happen. It's only when I run something like this:
`FUZZ=crypter src/test/fuzz/fuzz -set_cover_merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0 ../qa-assets/fuzz_corpus_copy/crypter ../qa-assets/fuzz_corpus_copy/*`
Then it continuously fails to merge and that bad alloc error keeps coming up over and over. I tried this same command on other harnesses that contain `
...
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2118863790)
If I don't start with a seed corpus (inputs from scratch) then it seems to run fine. I let it go for a while and no crashes happen. It's only when I run something like this:
`FUZZ=crypter src/test/fuzz/fuzz -set_cover_merge=1 -shuffle=0 -prefer_small=1 -use_value_profile=0 ../qa-assets/fuzz_corpus_copy/crypter ../qa-assets/fuzz_corpus_copy/*`
Then it continuously fails to merge and that bad alloc error keeps coming up over and over. I tried this same command on other harnesses that contain `
...
💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2118869082)
> set_cover_merge
This uses more memory than `-merge=1`, so my recommendation would be to try to add more memory or swap to your machine, or to use `-merge=1` for now.
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-2118869082)
> set_cover_merge
This uses more memory than `-merge=1`, so my recommendation would be to try to add more memory or swap to your machine, or to use `-merge=1` for now.
💬 jonatack commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605827914)
Hm, I don't think so, as the sentence you mention refers to Linux distributions: `Please be aware that on Linux distributions all dependencies are usually available as packages, but could be outdated.` This sentence refers to calling the files directly, so it's added in the section where it's relevant.
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605827914)
Hm, I don't think so, as the sentence you mention refers to Linux distributions: `Please be aware that on Linux distributions all dependencies are usually available as packages, but could be outdated.` This sentence refers to calling the files directly, so it's added in the section where it's relevant.
✅ hebasto closed a pull request: "build: Disable `_FORTIFY_SOURCE` if using MSan"
(https://github.com/bitcoin/bitcoin/pull/30140)
(https://github.com/bitcoin/bitcoin/pull/30140)
💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605878784)
The dependency version is picked at install time, and the previous section is about installing the dependencies, so it seems more suitable.
But just a nit, either is fine.
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605878784)
The dependency version is picked at install time, and the previous section is about installing the dependencies, so it seems more suitable.
But just a nit, either is fine.
💬 Geremia commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118963106)
@maflcko
> The debug log says "Shutdown: done", but you claim that "It connects to many peers", indicating that the program is still running?
When I restart bitcoin-qt, it should have asked to reindex, but instead it connects to peers and tries to sync (but stalls):
The debug log doesn't show any more corrupt DB errors than the one I pasted above.
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118963106)
@maflcko
> The debug log says "Shutdown: done", but you claim that "It connects to many peers", indicating that the program is still running?
When I restart bitcoin-qt, it should have asked to reindex, but instead it connects to peers and tries to sync (but stalls):
The debug log doesn't show any more corrupt DB errors than the one I pasted above.
💬 AngusP commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118985433)
tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
I did wonder whether `maxorphantx` was zeroed or otherwise not the default `100` for functests which would perhaps explain it being set here, but I can't see anything to indicate that being the case.
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118985433)
tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
I did wonder whether `maxorphantx` was zeroed or otherwise not the default `100` for functests which would perhaps explain it being set here, but I can't see anything to indicate that being the case.
👍 AngusP approved a pull request: "test: remove unneeded `-maxorphantx=1000` settings"
(https://github.com/bitcoin/bitcoin/pull/30133#pullrequestreview-2064941827)
tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
I did wonder whether `maxorphantx` was zeroed or otherwise not the default `100` for functests which would perhaps explain it being set here, but I can't see anything to indicate that being the case.
(https://github.com/bitcoin/bitcoin/pull/30133#pullrequestreview-2064941827)
tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
I did wonder whether `maxorphantx` was zeroed or otherwise not the default `100` for functests which would perhaps explain it being set here, but I can't see anything to indicate that being the case.
💬 AngusP commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#discussion_r1605886064)
nit:
```python
self.extra_args = [
[],
...
```
leaving
```python
self.extra_args = [
[
],
...
```
is a bit weird
(https://github.com/bitcoin/bitcoin/pull/30133#discussion_r1605886064)
nit:
```python
self.extra_args = [
[],
...
```
leaving
```python
self.extra_args = [
[
],
...
```
is a bit weird
💬 edilmedeiros commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605887608)
> Would word it as "Code needs to adhere to the C++20 standard.".
I second this.
(https://github.com/bitcoin/bitcoin/pull/30136#discussion_r1605887608)
> Would word it as "Code needs to adhere to the C++20 standard.".
I second this.
💬 Geremia commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118987509)
I also noticed that from `2024-05-18T04:48:21Z` to `2024-05-18T04:48:35Z` (14 second duration) I got 30 messages in `debug.log` that say:
```
2024-05-18T04:48:21Z UpdateTip: new best=00000000000000000003483012d42cbcd1ebe486364e46a56b8800fadfd43b74 height=843741 version=0x20f12000 log2_work=94.927416 tx=1005741855 date='2024-05-16T18:51:28Z' progress=0.999244 cache=1.4MiB(11882txo) warning='Miner violated version bit protocol'
2024-05-18T04:48:22Z UpdateTip: new best=0000000000000000000014c2b9
...
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118987509)
I also noticed that from `2024-05-18T04:48:21Z` to `2024-05-18T04:48:35Z` (14 second duration) I got 30 messages in `debug.log` that say:
```
2024-05-18T04:48:21Z UpdateTip: new best=00000000000000000003483012d42cbcd1ebe486364e46a56b8800fadfd43b74 height=843741 version=0x20f12000 log2_work=94.927416 tx=1005741855 date='2024-05-16T18:51:28Z' progress=0.999244 cache=1.4MiB(11882txo) warning='Miner violated version bit protocol'
2024-05-18T04:48:22Z UpdateTip: new best=0000000000000000000014c2b9
...