💬 polespinasa commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3234490515)
> It is just dead code now, no?
@maflcko I think you're right.
Will squash after review, think it's easier this way.
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3234490515)
> It is just dead code now, no?
@maflcko I think you're right.
Will squash after review, think it's easier this way.
💬 davidgumberg commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308207185)
In https://github.com/bitcoin/bitcoin/pull/32159/commits/42e99ad77396e4e9b02d67daf46349e215e99a0f (_net: skip non-route netlink responses_)
The `man 7 rtnetlink` page (https://man7.org/linux/man-pages/man7/rtnetlink.7.html) does not really offer much insight here, but as far as I can tell, the convention comes from the fact that the rtnetlink interface is also used for monitoring changes to the kernel's routing table, and because the userspace application is trying to maintain an idea of the
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308207185)
In https://github.com/bitcoin/bitcoin/pull/32159/commits/42e99ad77396e4e9b02d67daf46349e215e99a0f (_net: skip non-route netlink responses_)
The `man 7 rtnetlink` page (https://man7.org/linux/man-pages/man7/rtnetlink.7.html) does not really offer much insight here, but as far as I can tell, the convention comes from the fact that the rtnetlink interface is also used for monitoring changes to the kernel's routing table, and because the userspace application is trying to maintain an idea of the
...
💬 polespinasa commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#discussion_r2308221628)
forgot to remove the line, will do next push
(https://github.com/bitcoin/bitcoin/pull/32138#discussion_r2308221628)
forgot to remove the line, will do next push
🚀 fanquake merged a pull request: "ci: return to using dash in CentOS job"
(https://github.com/bitcoin/bitcoin/pull/33261)
(https://github.com/bitcoin/bitcoin/pull/33261)
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308229142)
done
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308229142)
done
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308230173)
I like to see `false` if it's not in b validation.
Fixed the "estimate of"
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308230173)
I like to see `false` if it's not in b validation.
Fixed the "estimate of"
🚀 fanquake merged a pull request: "threading: reduce the scope of lock in getblocktemplate"
(https://github.com/bitcoin/bitcoin/pull/33264)
(https://github.com/bitcoin/bitcoin/pull/33264)
🚀 fanquake merged a pull request: "rpc: followups for 33106"
(https://github.com/bitcoin/bitcoin/pull/33189)
(https://github.com/bitcoin/bitcoin/pull/33189)
🚀 fanquake merged a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258)
(https://github.com/bitcoin/bitcoin/pull/33258)
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308265935)
That's a good point, but that's actually what we are showing in the logs. Do we want to keep consistency between both? Update maybe also the logging?
```[background validation] UpdateTip: new best=00000000000000000007b796179f5abb7eed59f736e3418eb8381c29eb78527a height=744000 version=0x20400004 log2_work=93.616004 tx=747106639 date='2022-07-07T14:47:25Z' progress=0.605933 cache=358.0MiB(2606151txo)```
In `validation.cpp`
```c++
constexpr int BACKGROUND_LOG_INTERVAL = 2000;
if
...
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308265935)
That's a good point, but that's actually what we are showing in the logs. Do we want to keep consistency between both? Update maybe also the logging?
```[background validation] UpdateTip: new best=00000000000000000007b796179f5abb7eed59f736e3418eb8381c29eb78527a height=744000 version=0x20400004 log2_work=93.616004 tx=747106639 date='2022-07-07T14:47:25Z' progress=0.605933 cache=358.0MiB(2606151txo)```
In `validation.cpp`
```c++
constexpr int BACKGROUND_LOG_INTERVAL = 2000;
if
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376222)
Done
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376222)
Done
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376554)
You're correct - the comments were misleading. I've updated them to accurately reflect that MiniWallet uses consistent fee behavior for all transactions. The test still validates `getblockstats` functionality across different block structures, which is the primary goal.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376554)
You're correct - the comments were misleading. I've updated them to accurately reflect that MiniWallet uses consistent fee behavior for all transactions. The test still validates `getblockstats` functionality across different block structures, which is the primary goal.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376622)
You're correct that UTXO statistics don't depend on output destination. However, I'm keeping external outputs to maintain consistency with the original test's transaction structure. Changing to `send_self_transfer` would alter the generated test data and expected statistics values.
Since the goal is to replace the wallet implementation while preserving test behavior, `send_to` with external scripts is the most faithful translation of the original `sendtoaddress` calls.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376622)
You're correct that UTXO statistics don't depend on output destination. However, I'm keeping external outputs to maintain consistency with the original test's transaction structure. Changing to `send_self_transfer` would alter the generated test data and expected statistics values.
Since the goal is to replace the wallet implementation while preserving test behavior, `send_to` with external scripts is the most faithful translation of the original `sendtoaddress` calls.
💬 w0xlt commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3234759432)
> IMO this warning is sufficient, but if you keep the error approach, consider removing the warning.
Thanks for the review @naiyoma .
The error will occur regardless, whether in master or in this PR.
This PR simply provides a clearer and more useful message.
Regarding removing the warning, I think the `bind` validation could be moved to occur before the onion validation. Good catch.
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3234759432)
> IMO this warning is sufficient, but if you keep the error approach, consider removing the warning.
Thanks for the review @naiyoma .
The error will occur regardless, whether in master or in this PR.
This PR simply provides a clearer and more useful message.
Regarding removing the warning, I think the `bind` validation could be moved to occur before the onion validation. Good catch.
✅ hebasto closed a pull request: "cmake, guix: Skip building tests in subtrees for releases"
(https://github.com/bitcoin/bitcoin/pull/32054)
(https://github.com/bitcoin/bitcoin/pull/32054)
🤔 hebasto reviewed a pull request: "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2"
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3166350833)
My Guix build on RISC-V hardware for some older version of this branch:
```
riscv64
62fa84fe6a59109f6f0e4e6bc99b818aceaa339c2928384379070fa0467b94a9 guix-build-663fb8bf44aa/output/aarch64-linux-gnu/SHA256SUMS.part
36d0d70798970aa734aa945523593acf9dd57fa461e0799f2c8b55d4c4881fdd guix-build-663fb8bf44aa/output/aarch64-linux-gnu/bitcoin-663fb8bf44aa-aarch64-linux-gnu-debug.tar.gz
3852c55e6cd103f3db42e7a111b2885574956aa38bed202bc0669e261dc52c59 guix-build-663fb8bf44aa/output/aarch64-linux-gn
...
(https://github.com/bitcoin/bitcoin/pull/33185#pullrequestreview-3166350833)
My Guix build on RISC-V hardware for some older version of this branch:
```
riscv64
62fa84fe6a59109f6f0e4e6bc99b818aceaa339c2928384379070fa0467b94a9 guix-build-663fb8bf44aa/output/aarch64-linux-gnu/SHA256SUMS.part
36d0d70798970aa734aa945523593acf9dd57fa461e0799f2c8b55d4c4881fdd guix-build-663fb8bf44aa/output/aarch64-linux-gnu/bitcoin-663fb8bf44aa-aarch64-linux-gnu-debug.tar.gz
3852c55e6cd103f3db42e7a111b2885574956aa38bed202bc0669e261dc52c59 guix-build-663fb8bf44aa/output/aarch64-linux-gn
...
💬 davidgumberg commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308434775)
> Is it guaranteed that the reponse to `NLM_F_DUMP` is always multipart? Or do we need to check `nlmsg_flags` for `NLM_F_MULTI`, and if not, break after the first packet?
It seems that in the linux kernel, that is the case, a message type's (e.g. GETROUTE) `control` callback's `[.dumpit]`(https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c29/include/net/rtnetlink.h#L50) is [called](https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c29/net/n
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308434775)
> Is it guaranteed that the reponse to `NLM_F_DUMP` is always multipart? Or do we need to check `nlmsg_flags` for `NLM_F_MULTI`, and if not, break after the first packet?
It seems that in the linux kernel, that is the case, a message type's (e.g. GETROUTE) `control` callback's `[.dumpit]`(https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c29/include/net/rtnetlink.h#L50) is [called](https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c29/net/n
...
💬 fqlx commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#issuecomment-3234803555)
> I expect those to be widely used (not only in the internal tests, as you noticed in the repeated force pushes), so this may or may not be a widely breaking change (depending on how much they are used in active projects).
>
> There is no auto-generated schema or RPC interface, so those projects would have to fix them manually.
>
> No strong opinion, but maybe this can be closed for now, and revisited when there is a schema generator?
In my opinion, this is a significantly breaking chan
...
(https://github.com/bitcoin/bitcoin/pull/33214#issuecomment-3234803555)
> I expect those to be widely used (not only in the internal tests, as you noticed in the repeated force pushes), so this may or may not be a widely breaking change (depending on how much they are used in active projects).
>
> There is no auto-generated schema or RPC interface, so those projects would have to fix them manually.
>
> No strong opinion, but maybe this can be closed for now, and revisited when there is a schema generator?
In my opinion, this is a significantly breaking chan
...
💬 achow101 commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3234820665)
I've done some experimenting with a test project on transifex to see what the actual effects this change would cause, and a potential workaround.
In this test project, I copied over a couple fully translated languages to observe how the translated strings change when the source is updated. After rebasing this PR branch and doing `cmake --build build --target translate`, I uploaded the resulting modified `bitcoin_en.xlf` file. This resulted in 122 (~10% of strings) being marked untranslated ag
...
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3234820665)
I've done some experimenting with a test project on transifex to see what the actual effects this change would cause, and a potential workaround.
In this test project, I copied over a couple fully translated languages to observe how the translated strings change when the source is updated. After rebasing this PR branch and doing `cmake --build build --target translate`, I uploaded the resulting modified `bitcoin_en.xlf` file. This resulted in 122 (~10% of strings) being marked untranslated ag
...
💬 achow101 commented on issue "Zero output not cleared":
(https://github.com/bitcoin/bitcoin/issues/33265#issuecomment-3234847341)
This seems like it's because the check for whether a transaction is "from" the wallet is if it a non-zero amount leaves the wallet. Since these outputs are 0 value, they are definitionally do not meet that criteria.
(https://github.com/bitcoin/bitcoin/issues/33265#issuecomment-3234847341)
This seems like it's because the check for whether a transaction is "from" the wallet is if it a non-zero amount leaves the wallet. Since these outputs are 0 value, they are definitionally do not meet that criteria.