💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2673834899)
`69e076664d...76feae207d`: rebase due to conflicts and allow to log but not treat as an error detected internet traffic (`$INTERNET_TRAFFIC_EXPECTED` mentioned above). Can be used when running manually outside of CI where other programs on the host can generate internet traffic.
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2673834899)
`69e076664d...76feae207d`: rebase due to conflicts and allow to log but not treat as an error detected internet traffic (`$INTERNET_TRAFFIC_EXPECTED` mentioned above). Can be used when running manually outside of CI where other programs on the host can generate internet traffic.
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2673838834)
re-ACK 61885ad9d406e25ec3b6523d01918e886996f1c3
Dropped the else: https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2673838834)
re-ACK 61885ad9d406e25ec3b6523d01918e886996f1c3
Dropped the else: https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1963881147
💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2673844440)
> I think it is a little bit unfortunate that there might be some users of the GUI who don't check the release notes or debug.log whose networks did support UPnP but don't support NAT-PMP or PCP will never be warned that their port forwarding setup is broken
> Maybe a better and more general solution to that class of problem that is outside the scope of this PR and the issue it resolves is to provide some kind of indication to users about whether or not their node is reachable, maybe using a
...
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2673844440)
> I think it is a little bit unfortunate that there might be some users of the GUI who don't check the release notes or debug.log whose networks did support UPnP but don't support NAT-PMP or PCP will never be warned that their port forwarding setup is broken
> Maybe a better and more general solution to that class of problem that is outside the scope of this PR and the issue it resolves is to provide some kind of indication to users about whether or not their node is reachable, maybe using a
...
💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965038094)
No, that's not intended. i'll look into a way to query the json only.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965038094)
No, that's not intended. i'll look into a way to query the json only.
💬 maflcko commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2673868944)
> If we had an icon in the GUI that showed the current port forwarding status (or as you say, whether there are incoming connections), this would be solved too. But that's something for a future PR.
IIRC I designed the "connections" icon so that it shows 3 "arms" when there are `8` connections, thus likely no incoming ones. Though, this probably broke with block-relay-only connections and it now shows always 4 "arms". One could address that, to have that approximation again, but as you say it
...
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2673868944)
> If we had an icon in the GUI that showed the current port forwarding status (or as you say, whether there are incoming connections), this would be solved too. But that's something for a future PR.
IIRC I designed the "connections" icon so that it shows 3 "arms" when there are `8` connections, thus likely no incoming ones. Though, this probably broke with block-relay-only connections and it now shows always 4 "arms". One could address that, to have that approximation again, but as you say it
...
💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2673871527)
Rebased and dropped coinbase from `vTxFees` and `vTxSigOpsCost`.
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2673871527)
Rebased and dropped coinbase from `vTxFees` and `vTxSigOpsCost`.
💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965052114)
Thank you!
`-upnp=0` setting the default for `-natpmp` to false sounds like reasonable behavior.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965052114)
Thank you!
`-upnp=0` setting the default for `-natpmp` to false sounds like reasonable behavior.
💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2673877626)
@pinheadmz you can even run ad-hoc signed binaries when you scp them.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2673877626)
@pinheadmz you can even run ad-hoc signed binaries when you scp them.
💬 maflcko commented on issue "Fuzzing Bitcoin Core with clang-16":
(https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781)
> [ 28%] Linking CXX executable bitcoin-wallet
There is no such executable in the fuzz build target. When using cmake, you will have to delete the build folder before configuring. Otherwise the build could be stale.
Also, your system has clang-19 available to use, see https://packages.debian.org/bookworm/clang-19
What are the exact steps to reproduce?
I can't reproduce this locally with:
```
rm -rf ./bld-fzz && cmake -DCMAKE_C_COMPILER=/usr/bin/clang-16 -DCMAKE_CXX_COMPILER=/usr/bin/clang-
...
(https://github.com/bitcoin/bitcoin/issues/31922#issuecomment-2673888781)
> [ 28%] Linking CXX executable bitcoin-wallet
There is no such executable in the fuzz build target. When using cmake, you will have to delete the build folder before configuring. Otherwise the build could be stale.
Also, your system has clang-19 available to use, see https://packages.debian.org/bookworm/clang-19
What are the exact steps to reproduce?
I can't reproduce this locally with:
```
rm -rf ./bld-fzz && cmake -DCMAKE_C_COMPILER=/usr/bin/clang-16 -DCMAKE_CXX_COMPILER=/usr/bin/clang-
...
💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2673903627)
> > An earlier Apple Forum thread also suggests that the way to notarize these binaries is to put the contents of the tar.gz archive into a zip and then upload
>
> That's what we're doing.
Maybe I'm misreading the `find` incantation here:
```cpp
# Notarize the binaries
# Binaries cannot have stapled notarizations so this does not actually generate any output
binaries_dir=$(dirname "$(find . -maxdepth 2 -wholename '*/bin' -type d -exec realpath --relative-to=. {} \;)")
${SIGNAPPLE} n
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2673903627)
> > An earlier Apple Forum thread also suggests that the way to notarize these binaries is to put the contents of the tar.gz archive into a zip and then upload
>
> That's what we're doing.
Maybe I'm misreading the `find` incantation here:
```cpp
# Notarize the binaries
# Binaries cannot have stapled notarizations so this does not actually generate any output
binaries_dir=$(dirname "$(find . -maxdepth 2 -wholename '*/bin' -type d -exec realpath --relative-to=. {} \;)")
${SIGNAPPLE} n
...
💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2673922683)
I remember the arms feature being very useful for exactly that reason, but haven't tried recently.
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2673922683)
I remember the arms feature being very useful for exactly that reason, but haven't tried recently.
💬 l0rinc commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1965091651)
> Someone checked that the fuzz engine can break the checksum (ideal).
It probably can, eventually, but ran it overnight with:
```C++
const int max_ret_len{provider.ConsumeIntegralInRange<int>(-1, 100)};
if (std::vector<unsigned char> decoded; DecodeBase58Check(random_string, decoded, max_ret_len)) {
throw "cracked!" + random_string;
}
```
It ran 134 million executions and it didn't fail - my new PR should fix that, thanks for the observation
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1965091651)
> Someone checked that the fuzz engine can break the checksum (ideal).
It probably can, eventually, but ran it overnight with:
```C++
const int max_ret_len{provider.ConsumeIntegralInRange<int>(-1, 100)};
if (std::vector<unsigned char> decoded; DecodeBase58Check(random_string, decoded, max_ret_len)) {
throw "cracked!" + random_string;
}
```
It ran 134 million executions and it didn't fail - my new PR should fix that, thanks for the observation
💬 jsarenik commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2673958951)
> @jsarenik read the PR description https://github.com/bitcoin/bitcoin/pull/29838#issue-2233837348
Yes, it does not seem to honor the main signet. NACK
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2673958951)
> @jsarenik read the PR description https://github.com/bitcoin/bitcoin/pull/29838#issue-2233837348
Yes, it does not seem to honor the main signet. NACK
💬 dergoegge commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2673960681)
> You explicitly set it to the empty string in -DCMAKE_CXX_FLAGS_RELWITHDEBINFO="" and IIRC the default for compilers is -O0, so I presumed that this was intentional.
Ah yes. The actual setup involved setting flags like we do in oss-fuzz https://github.com/google/oss-fuzz/blob/1472130318ff69f8dcfc44db7aa9fe0610423c57/projects/bitcoin-core/build.sh#L70. So it doesn't work with `O0` or `O2`.
I wonder if the afl++ build failure is also why we don't see any afl++ fuzzing on oss-fuzz anymore (https
...
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2673960681)
> You explicitly set it to the empty string in -DCMAKE_CXX_FLAGS_RELWITHDEBINFO="" and IIRC the default for compilers is -O0, so I presumed that this was intentional.
Ah yes. The actual setup involved setting flags like we do in oss-fuzz https://github.com/google/oss-fuzz/blob/1472130318ff69f8dcfc44db7aa9fe0610423c57/projects/bitcoin-core/build.sh#L70. So it doesn't work with `O0` or `O2`.
I wonder if the afl++ build failure is also why we don't see any afl++ fuzzing on oss-fuzz anymore (https
...
💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965115244)
Why would `value.isTrue` fail? I suspect what will happen (still have to test) is that you'll keep `upnp=1` in the config and _also_ have `natpmp: 1` in settings. If the former is ignored and the latter used, that should be fine.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965115244)
Why would `value.isTrue` fail? I suspect what will happen (still have to test) is that you'll keep `upnp=1` in the config and _also_ have `natpmp: 1` in settings. If the former is ignored and the latter used, that should be fine.
💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965102891)
We might want to wait longer, because people can upgrade many years later (e.g. because they suddenly remember they have a wallet). Maybe just drop the TODO.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965102891)
We might want to wait longer, because people can upgrade many years later (e.g. because they suddenly remember they have a wallet). Maybe just drop the TODO.
👍 dergoegge approved a pull request: "fuzz: add basic TxOrphanage::EraseForBlock cov"
(https://github.com/bitcoin/bitcoin/pull/31918#pullrequestreview-2632437674)
utACK 8400b742fa6dda4ad89311f547ccf50b0187e817
(https://github.com/bitcoin/bitcoin/pull/31918#pullrequestreview-2632437674)
utACK 8400b742fa6dda4ad89311f547ccf50b0187e817
💬 maflcko commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2673989827)
> I wonder if the afl++ build failure is also why we don't see any afl++ fuzzing on oss-fuzz anymore ([google/oss-fuzz#13020](https://github.com/google/oss-fuzz/issues/13020)).
oss-fuzz has -O1 set, so I think the build should succeed. See also https://oss-fuzz-build-logs.storage.googleapis.com/index.html#bitcoin-core
Excerpt:
```
Step #3 - "compile-afl-address-x86_64": [0mC++ compiler .......................... Clang 18.1.8, /src/aflplusplus/afl-clang-fast++ [0m
Step #3 - "compile-afl-addre
...
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2673989827)
> I wonder if the afl++ build failure is also why we don't see any afl++ fuzzing on oss-fuzz anymore ([google/oss-fuzz#13020](https://github.com/google/oss-fuzz/issues/13020)).
oss-fuzz has -O1 set, so I think the build should succeed. See also https://oss-fuzz-build-logs.storage.googleapis.com/index.html#bitcoin-core
Excerpt:
```
Step #3 - "compile-afl-address-x86_64": [0mC++ compiler .......................... Clang 18.1.8, /src/aflplusplus/afl-clang-fast++ [0m
Step #3 - "compile-afl-addre
...
💬 dergoegge commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2674013479)
For me the `afl-clang-{fast,lto}` build fails even with `-O1` but you're right the oss-fuzz build logs look fine. I'll try using the afl++ version they use, maybe there's a bug in the newer version I use. In any case the afl++ issue seems separate from the secp asm failure with plain clang (even though the error message is almost the same).
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2674013479)
For me the `afl-clang-{fast,lto}` build fails even with `-O1` but you're right the oss-fuzz build logs look fine. I'll try using the afl++ version they use, maybe there's a bug in the newer version I use. In any case the afl++ issue seems separate from the secp asm failure with plain clang (even though the error message is almost the same).
💬 Sjors commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965129113)
`in bitcoin.conf or settings.json, replacing it with -natpmp in settings.json`
When it's in `bitcoin.conf` the warning will get repeated every startup, which seems fine.
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965129113)
`in bitcoin.conf or settings.json, replacing it with -natpmp in settings.json`
When it's in `bitcoin.conf` the warning will get repeated every startup, which seems fine.