💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995454609)
I was trying to say in https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986292978 that a default doesn't make sense at all here (in any way), because:
* Every call site that omits the default has the same fallback, so changing the default in the future will make review harder.
* Every call site needs to document the default in the RPC help anyway, so simply using the default from the docs is self-consistent and self-documenting and avoids the need for a default here
Finally, def
...
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1995454609)
I was trying to say in https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1986292978 that a default doesn't make sense at all here (in any way), because:
* Every call site that omits the default has the same fallback, so changing the default in the future will make review harder.
* Every call site needs to document the default in the RPC help anyway, so simply using the default from the docs is self-consistent and self-documenting and avoids the need for a default here
Finally, def
...
⚠️ polespinasa opened an issue: "Doc: Mempool Policy documentation Outdated since TRUC"
(https://github.com/bitcoin/bitcoin/issues/32067)
Checking the [policy documentation](https://github.com/bitcoin/bitcoin/tree/master/doc/policy) I think I noticed that some things are outdated since Version 3 transaction relay / TRUC.
Some sentences [like](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md#package-fees-and-feerate):
> Note: Package feerate cannot be used to meet the minimum relay feerate (-minrelaytxfee) requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is se
...
(https://github.com/bitcoin/bitcoin/issues/32067)
Checking the [policy documentation](https://github.com/bitcoin/bitcoin/tree/master/doc/policy) I think I noticed that some things are outdated since Version 3 transaction relay / TRUC.
Some sentences [like](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/packages.md#package-fees-and-feerate):
> Note: Package feerate cannot be used to meet the minimum relay feerate (-minrelaytxfee) requirement. For example, if the mempool minimum feerate is 5sat/vB and the minimum relay feerate is se
...
💬 hodlinator commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995495622)
[Seems](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/setting-a-default-shell-and-working-directory#example-set-the-default-shell-and-working-directory) like we might be able override default shell at the top of ci.yml file:
```
defaults:
run:
shell: bash
```
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1995495622)
[Seems](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/setting-a-default-shell-and-working-directory#example-set-the-default-shell-and-working-directory) like we might be able override default shell at the top of ci.yml file:
```
defaults:
run:
shell: bash
```
💬 maflcko commented on pull request "test: Rename send_message to send_without_ping":
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2724589926)
rebased and taken out of draft. This should be ready for review, as all conflicts are draft pull requests.
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2724589926)
rebased and taken out of draft. This should be ready for review, as all conflicts are draft pull requests.
💬 laanwj commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724599518)
Concept ACK, nice work
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724599518)
Concept ACK, nice work
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995506801)
Dropped changes to `depends/hosts/linux.mk`.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995506801)
Dropped changes to `depends/hosts/linux.mk`.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995510268)
I didn't realize that these flags are used also for the libraries. Dropped these changes.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995510268)
I didn't realize that these flags are used also for the libraries. Dropped these changes.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995511934)
Given the above I left non-debug builds alone.
(https://github.com/bitcoin/bitcoin/pull/31424#discussion_r1995511934)
Given the above I left non-debug builds alone.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724614671)
`8cf75a4...a3a799c`: reduce the scope of this to only add hardening on libc++ when in debug mode, updated the OP.
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724614671)
`8cf75a4...a3a799c`: reduce the scope of this to only add hardening on libc++ when in debug mode, updated the OP.
👋 vasild's pull request is ready for review: "build: enable libc++ hardening"
(https://github.com/bitcoin/bitcoin/pull/31424)
(https://github.com/bitcoin/bitcoin/pull/31424)
💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2724627981)
Reviewed the first four commits up to 0d6e10674ea8578ddcf75318b25e86dee430bc09.
- I've been rebasing #30975 and #31802 on top of this for increased CI coverage and Guix builds. Initially this found several bugs / issues, but recently it's been smooth sailing.
- I ran `git-subtree-check.sh` (see PR description)
- the scripted diff to use `ENABLE_IPC` is trivial
- I tested c27d3a710b844e845075dea7e51f8f368ebf409f "cmake: Support building with libmultiprocess subtree" on macOS 15.3.2 by build
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2724627981)
Reviewed the first four commits up to 0d6e10674ea8578ddcf75318b25e86dee430bc09.
- I've been rebasing #30975 and #31802 on top of this for increased CI coverage and Guix builds. Initially this found several bugs / issues, but recently it's been smooth sailing.
- I ran `git-subtree-check.sh` (see PR description)
- the scripted diff to use `ENABLE_IPC` is trivial
- I tested c27d3a710b844e845075dea7e51f8f368ebf409f "cmake: Support building with libmultiprocess subtree" on macOS 15.3.2 by build
...
💬 maflcko commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724631464)
> Further considerations:
>
> * Consider enabling `libstdc++`s `_GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC` in debug builds. The problem is that ["Note that this flag changes the sizes and behavior of standard class templates such as std::vector, and therefore you can only link code compiled with debug mode and code compiled without debug mode if no instantiation of a container is passed between the two translation units."](https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html#d
...
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724631464)
> Further considerations:
>
> * Consider enabling `libstdc++`s `_GLIBCXX_DEBUG _GLIBCXX_DEBUG_PEDANTIC` in debug builds. The problem is that ["Note that this flag changes the sizes and behavior of standard class templates such as std::vector, and therefore you can only link code compiled with debug mode and code compiled without debug mode if no instantiation of a container is passed between the two translation units."](https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_using.html#d
...
💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1995536108)
hi @furszy can you please check the fix.
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1995536108)
hi @furszy can you please check the fix.
💬 vasild commented on pull request "build: enable libc++ hardening":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724652160)
Excellent! Removed from OP.
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2724652160)
Excellent! Removed from OP.
⚠️ Sjors opened an issue: "depends: capnp build ignores config_opts"
(https://github.com/bitcoin/bitcoin/issues/32068)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
It seems that when building capnp via depends the `config_opts` in `capnp.mk` are ignored, i.e. `-DBUILD_TESTING=OFF`, `-DWITH_OPENSSL=OFF` and `-DWITH_ZLIB=OFF`.
This can be seen in the depends build log by e.g. `Built target capnp-heavy-tests`, a target that should be skipped entirely. It becomes a problem during the `cmake -B build --toolchain ...` step, because it will complain about
...
(https://github.com/bitcoin/bitcoin/issues/32068)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
It seems that when building capnp via depends the `config_opts` in `capnp.mk` are ignored, i.e. `-DBUILD_TESTING=OFF`, `-DWITH_OPENSSL=OFF` and `-DWITH_ZLIB=OFF`.
This can be seen in the depends build log by e.g. `Built target capnp-heavy-tests`, a target that should be skipped entirely. It becomes a problem during the `cmake -B build --toolchain ...` step, because it will complain about
...
💬 l0rinc commented on pull request "[IBD] flush UXTOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2724691617)
Reran the benchmarks on a Hetzner Linux machine with the new AssumeUTXO 880k set, parsing the logged flush times and plotting the results.
For different `dbcache` (440, 5000, 30000) and `dbbatchsize` (4-256MiB range, 16MiB (current) and 64MiB (proposed) are highlighted and trendline is added for clarity) sizes, each one twice for stability:
Sorting the same measurements might give us a better understanding of the trends:
<img width="1000" alt="image" src="https://github.com/user-attachments
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2724691617)
Reran the benchmarks on a Hetzner Linux machine with the new AssumeUTXO 880k set, parsing the logged flush times and plotting the results.
For different `dbcache` (440, 5000, 30000) and `dbbatchsize` (4-256MiB range, 16MiB (current) and 64MiB (proposed) are highlighted and trendline is added for clarity) sizes, each one twice for stability:
Sorting the same measurements might give us a better understanding of the trends:
<img width="1000" alt="image" src="https://github.com/user-attachments
...
💬 pinheadmz commented on pull request "build: Remove manpages when making MacOS app":
(https://github.com/bitcoin/bitcoin/pull/32064#issuecomment-2724739546)
post-merge tested ACK
successful guix build, code sign, and signature attach from current master, ran Qt and bitcoin on arm64/macos.
Didn't test x86: https://github.com/pinheadmz/bitcoin-detached-sigs/tree/master-698f86964c
(https://github.com/bitcoin/bitcoin/pull/32064#issuecomment-2724739546)
post-merge tested ACK
successful guix build, code sign, and signature attach from current master, ran Qt and bitcoin on arm64/macos.
Didn't test x86: https://github.com/pinheadmz/bitcoin-detached-sigs/tree/master-698f86964c
💬 Sjors commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724754746)
The macOS native builds don't use depends, so #30975 would not pick this is up.
cc @ryanofsky
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2724754746)
The macOS native builds don't use depends, so #30975 would not pick this is up.
cc @ryanofsky
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995616163)
May I know what is the issue with current fix: It will handle negative and positive, "never" cases.
` auto requestTimestamp = GetImportTimestamp(request, now);
// if any one entity has valid timestamp we can skp this check
if (!all_rescan_value) {
if (requestTimestamp < 0) {
const UniValue& timestamp = request["timestamp"];
// set all_rescan_value true if requested timestamp is negative otherwise i
...
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1995616163)
May I know what is the issue with current fix: It will handle negative and positive, "never" cases.
` auto requestTimestamp = GetImportTimestamp(request, now);
// if any one entity has valid timestamp we can skp this check
if (!all_rescan_value) {
if (requestTimestamp < 0) {
const UniValue& timestamp = request["timestamp"];
// set all_rescan_value true if requested timestamp is negative otherwise i
...
💬 vasild commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724787234)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2724787234)
Concept ACK