💬 maflcko commented on pull request "CMake replaces Autotools":
(https://github.com/bitcoin/bitcoin/pull/30664#discussion_r1724536921)
style nit: If you are removing the only invocation of this script, wouldn't it be better to remove the other mentions in the docs as well?
```
cmake/script/GenerateBuildInfo.cmake:# This script is a multiplatform port of the share/genbuild.sh shell script.
src/clientversion.cpp:// The <obj/build.h>, which is generated by the build environment (share/genbuild.sh),
(https://github.com/bitcoin/bitcoin/pull/30664#discussion_r1724536921)
style nit: If you are removing the only invocation of this script, wouldn't it be better to remove the other mentions in the docs as well?
```
cmake/script/GenerateBuildInfo.cmake:# This script is a multiplatform port of the share/genbuild.sh shell script.
src/clientversion.cpp:// The <obj/build.h>, which is generated by the build environment (share/genbuild.sh),
💬 maflcko commented on pull request "CMake replaces Autotools":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2301321844)
> It aims reveal conflicts with other PRs.
>
> For more details, please refer to today's IRC meeting discussion.
It would be good to mention any important key points, otherwise it will be hard to understand whether this is the pull request that will be merged as a follow-up, and whether it should be reviewed, or if there is another place where the review should happen.
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2301321844)
> It aims reveal conflicts with other PRs.
>
> For more details, please refer to today's IRC meeting discussion.
It would be good to mention any important key points, otherwise it will be hard to understand whether this is the pull request that will be merged as a follow-up, and whether it should be reviewed, or if there is another place where the review should happen.
👋 maflcko's pull request is ready for review: "Bump python minimum supported version to 3.10"
(https://github.com/bitcoin/bitcoin/pull/30527)
(https://github.com/bitcoin/bitcoin/pull/30527)
💬 maflcko commented on pull request "Bump python minimum supported version to 3.10":
(https://github.com/bitcoin/bitcoin/pull/30527#issuecomment-2301329179)
Unlocked for review for 29.x
(https://github.com/bitcoin/bitcoin/pull/30527#issuecomment-2301329179)
Unlocked for review for 29.x
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301457862)
An O2 godbolt would be https://godbolt.org/z/Kqxe6353P
> +0.04% seems okay if you ask me.
Are you sure? I checked the DrahtBot guix build and everything I checked was smaller, but maybe the build is malicious or I made a mistake.
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301457862)
An O2 godbolt would be https://godbolt.org/z/Kqxe6353P
> +0.04% seems okay if you ask me.
Are you sure? I checked the DrahtBot guix build and everything I checked was smaller, but maybe the build is malicious or I made a mistake.
💬 fanquake commented on pull request "depends: Fix CMake-generated `libzmq.pc` file":
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301464512)
> I'd rather wait until upstream weighs in though, since we don't need this at least until we switch to CMake.
Looks like upstream either forgot about, or missed this, so I guess someone should followup.
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301464512)
> I'd rather wait until upstream weighs in though, since we don't need this at least until we switch to CMake.
Looks like upstream either forgot about, or missed this, so I guess someone should followup.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301467409)
This is how I checked the sizes of current tip vs base:
```
$ ls -al guix-build-c3fa29db1b05/distsrc-c3fa29db1b05-x86_64-linux-gnu/src/bitcoind
-rwxr-xr-x 1 hodlinator users 115048128 aug 20 14:10 guix-build-c3fa29db1b05/distsrc-c3fa29db1b05-x86_64-linux-gnu/src/bitcoind
$ ls -al guix-build-5fdbc8b4ee60/distsrc-5fdbc8b4ee60-x86_64-linux-gnu/src/bitcoind
-rwxr-xr-x 1 hodlinator users 114998544 aug 13 11:29 guix-build-5fdbc8b4ee60/distsrc-5fdbc8b4ee60-x86_64-linux-gnu/src/bitcoind
```
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301467409)
This is how I checked the sizes of current tip vs base:
```
$ ls -al guix-build-c3fa29db1b05/distsrc-c3fa29db1b05-x86_64-linux-gnu/src/bitcoind
-rwxr-xr-x 1 hodlinator users 115048128 aug 20 14:10 guix-build-c3fa29db1b05/distsrc-c3fa29db1b05-x86_64-linux-gnu/src/bitcoind
$ ls -al guix-build-5fdbc8b4ee60/distsrc-5fdbc8b4ee60-x86_64-linux-gnu/src/bitcoind
-rwxr-xr-x 1 hodlinator users 114998544 aug 13 11:29 guix-build-5fdbc8b4ee60/distsrc-5fdbc8b4ee60-x86_64-linux-gnu/src/bitcoind
```
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1724656502)
Heh, nice. I didn't know that string literal operator templates are possible since C++20.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1724656502)
Heh, nice. I didn't know that string literal operator templates are possible since C++20.
💬 l0rinc commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1724663372)
Valid points, thanks @glozow & @maflcko, dropped that commit.
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1724663372)
Valid points, thanks @glozow & @maflcko, dropped that commit.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301487768)
I see, so I guess you are counting the increase in the debug symbols, which seems plausible, given that the pull request includes refactoring changes to introduce more call stacks (vector->span conversions, as well as newly introduced array->vector conversions)
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301487768)
I see, so I guess you are counting the increase in the debug symbols, which seems plausible, given that the pull request includes refactoring changes to introduce more call stacks (vector->span conversions, as well as newly introduced array->vector conversions)
👍 TheCharlatan approved a pull request: "test: replace deprecated secp256k1 context flags usage"
(https://github.com/bitcoin/bitcoin/pull/30687#pullrequestreview-2250169083)
ACK 60055f1abc4b4ad5f66a2fcf2e61c65efc777036
(https://github.com/bitcoin/bitcoin/pull/30687#pullrequestreview-2250169083)
ACK 60055f1abc4b4ad5f66a2fcf2e61c65efc777036
💬 maflcko commented on pull request "depends: Fix CMake-generated `libzmq.pc` file":
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301509656)
Could rebase the upstream pull, which currently fails CI, to pick up the upstream CI fixes?
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301509656)
Could rebase the upstream pull, which currently fails CI, to pick up the upstream CI fixes?
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301546811)
Aha.. was assuming Guix builds weren't producing debug symbols.. do the non-debug archives under */output/* contain the release-binaries? Results after untaring them:
```
$ ls -al guix-build-c3fa29db1b05/output/x86_64-linux-gnu/bitcoin-c3fa29db1b05/bin/bitcoind
-rwxr-xr-x 1 hodlinator users 16509032 jan 1 1980 guix-build-c3fa29db1b05/output/x86_64-linux-gnu/bitcoin-c3fa29db1b05/bin/bitcoind
$ ls -al guix-build-5fdbc8b4ee60/output/x86_64-linux-gnu/bitcoin-5fdbc8b4ee60/bin/bitcoind
-r
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301546811)
Aha.. was assuming Guix builds weren't producing debug symbols.. do the non-debug archives under */output/* contain the release-binaries? Results after untaring them:
```
$ ls -al guix-build-c3fa29db1b05/output/x86_64-linux-gnu/bitcoin-c3fa29db1b05/bin/bitcoind
-rwxr-xr-x 1 hodlinator users 16509032 jan 1 1980 guix-build-c3fa29db1b05/output/x86_64-linux-gnu/bitcoin-c3fa29db1b05/bin/bitcoind
$ ls -al guix-build-5fdbc8b4ee60/output/x86_64-linux-gnu/bitcoin-5fdbc8b4ee60/bin/bitcoind
-r
...
📝 brunoerg opened a pull request: "fuzz: speed up addrman"
(https://github.com/bitcoin/bitcoin/pull/30688)
There is no need to iterate 10'000 times and
call `Add` 10'000 times at once (we only add
1000 addresses at once in practice). These values
were introduced in 214d9055acdd72189a2f415477ce472ca8db4191.
I see an avg of 40 exec/s on master for this target and 150 exec/s on this branch.
(https://github.com/bitcoin/bitcoin/pull/30688)
There is no need to iterate 10'000 times and
call `Add` 10'000 times at once (we only add
1000 addresses at once in practice). These values
were introduced in 214d9055acdd72189a2f415477ce472ca8db4191.
I see an avg of 40 exec/s on master for this target and 150 exec/s on this branch.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724756834)
`CheckIsEmpty()` seems a bit more future-proof. We'll check
- no requests for this peer
- no pending orphan resolutions for this peer
- no orphans for this peer
etc.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724756834)
`CheckIsEmpty()` seems a bit more future-proof. We'll check
- no requests for this peer
- no pending orphan resolutions for this peer
- no orphans for this peer
etc.
💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301622285)
I don't think that enabling CET for GCC alone will work. Shouldn't `binutils` and `glibc` also be CET-enabled?
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301622285)
I don't think that enabling CET for GCC alone will work. Shouldn't `binutils` and `glibc` also be CET-enabled?
💬 maflcko commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2301624135)
> we only add
> 1000 addresses at once in practice
Are you referring to `MAX_ADDR_TO_SEND`? If yes, it could make sense to clarify this in the description or even in a code comemnent, given that the variable name seems to indicate it is only used for sending, not for processing/adding.
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2301624135)
> we only add
> 1000 addresses at once in practice
Are you referring to `MAX_ADDR_TO_SEND`? If yes, it could make sense to clarify this in the description or even in a code comemnent, given that the variable name seems to indicate it is only used for sending, not for processing/adding.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724766037)
I called it `HaveMoreWork` since its purpose is to tell `ThreadMessageHandler()` if there is more work to do (doesn't matter what kind of work):
https://github.com/bitcoin/bitcoin/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/net_processing.cpp#L5083
https://github.com/bitcoin/bitcoin/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/net.cpp#L2902-L2903
`GetTxToReconsider`'s user is peerman, in `ProcessOrphanTx`.
I think scheduled reconsideration work could become more generic in th
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724766037)
I called it `HaveMoreWork` since its purpose is to tell `ThreadMessageHandler()` if there is more work to do (doesn't matter what kind of work):
https://github.com/bitcoin/bitcoin/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/net_processing.cpp#L5083
https://github.com/bitcoin/bitcoin/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/net.cpp#L2902-L2903
`GetTxToReconsider`'s user is peerman, in `ProcessOrphanTx`.
I think scheduled reconsideration work could become more generic in th
...
💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2301629827)
> I'd rather build up from from something like #30438 (and have a branch with similar changes).
Is that branch publicly available?
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2301629827)
> I'd rather build up from from something like #30438 (and have a branch with similar changes).
Is that branch publicly available?
💬 fanquake commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301637021)
> will work.
What do you mean by "work" here? This PR is just explicitly turning on one option in the compiler.
> Shouldn't binutils ... also be CET-enabled?
What is a CET-enbled binutils? If you mean it supporting CET functionality, then it is new enough.
> glibc also be CET-enabled?
Last time I looked, glibc will autodetect this based on the compiler (we could still enable it explictly, but it's not clear that is required).
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301637021)
> will work.
What do you mean by "work" here? This PR is just explicitly turning on one option in the compiler.
> Shouldn't binutils ... also be CET-enabled?
What is a CET-enbled binutils? If you mean it supporting CET functionality, then it is new enough.
> glibc also be CET-enabled?
Last time I looked, glibc will autodetect this based on the compiler (we could still enable it explictly, but it's not clear that is required).