🤔 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.
💬 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_r2308480089)
Relevant section of iproute2, if a response to getroute is not of type `RTM_NEWROUTE`, it's an error:
```c
static int iproute_get(int argc, char **argv)
// [... `Populate answer` ]
struct rtmsg *r = NLMSG_DATA(answer);
if (answer->nlmsg_type != RTM_NEWROUTE) {
fprintf(stderr, "Not a route?\n");
delete_json_obj();
free(answer);
return -1;
}
}
```
https://github.com/iproute2/iproute2/blob/3337e5013e168bcf4fab5f6518d1e4293a0a830b/ip/iproute.c#L2242-L2247
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308480089)
Relevant section of iproute2, if a response to getroute is not of type `RTM_NEWROUTE`, it's an error:
```c
static int iproute_get(int argc, char **argv)
// [... `Populate answer` ]
struct rtmsg *r = NLMSG_DATA(answer);
if (answer->nlmsg_type != RTM_NEWROUTE) {
fprintf(stderr, "Not a route?\n");
delete_json_obj();
free(answer);
return -1;
}
}
```
https://github.com/iproute2/iproute2/blob/3337e5013e168bcf4fab5f6518d1e4293a0a830b/ip/iproute.c#L2242-L2247
💬 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_r2308514117)
Relevant (editorialized) code from iproute2:
```c
if (tb[RTA_DST]) {
// [...if the route has a specific destination, e.g.] ip route add 192.168.0.5 via 192.168.0.4
} else if (r->rtm_dst_len) {
// [if the route has prefix has a length]
snprintf(b1, sizeof(b1), "0/%d ", r->rtm_dst_len);
} else {
// [if rtm_dst_len == 0]
strncpy(b1, "default", sizeof(b1));
}
https://github.com/iproute2/iproute2/blob/3337e5013e168bcf4fab5f6518d1e4293a0a830b/ip/iproute.
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308514117)
Relevant (editorialized) code from iproute2:
```c
if (tb[RTA_DST]) {
// [...if the route has a specific destination, e.g.] ip route add 192.168.0.5 via 192.168.0.4
} else if (r->rtm_dst_len) {
// [if the route has prefix has a length]
snprintf(b1, sizeof(b1), "0/%d ", r->rtm_dst_len);
} else {
// [if rtm_dst_len == 0]
strncpy(b1, "default", sizeof(b1));
}
https://github.com/iproute2/iproute2/blob/3337e5013e168bcf4fab5f6518d1e4293a0a830b/ip/iproute.
...
💬 icedmoca commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3234912770)
The most robust mitigation to nonce-grind disk-bloat attacks on low-difficulty Signets and derivative test networks is to elevate the effective difficulty target beyond the 2³² threshold, thereby rendering exhaustive nonce enumeration infeasible within realistic compute bounds, while simultaneously hardening node storage semantics by enforcing deduplication constraints on alternative block headers at a given height and chainwork level. In practice, this entails (i) tuning the consensus difficult
...
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3234912770)
The most robust mitigation to nonce-grind disk-bloat attacks on low-difficulty Signets and derivative test networks is to elevate the effective difficulty target beyond the 2³² threshold, thereby rendering exhaustive nonce enumeration infeasible within realistic compute bounds, while simultaneously hardening node storage semantics by enforcing deduplication constraints on alternative block headers at a given height and chainwork level. In practice, this entails (i) tuning the consensus difficult
...
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3234922944)
a425bff75203edf855ac4d8a2fea3a0a60978147 added snapshot height
fd504bb351 modify the way verification progress is calculated for background validation -> context: https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306215222
(https://github.com/bitcoin/bitcoin/pull/33259#issuecomment-3234922944)
a425bff75203edf855ac4d8a2fea3a0a60978147 added snapshot height
fd504bb351 modify the way verification progress is calculated for background validation -> context: https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2306215222
💬 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_r2308553353)
nit: should also error on `recv_result == 0`, e.g. from libnetlink:
```c
static int __rtnl_recvmsg(int fd, struct msghdr *msg, int flags)
{
int len;
do {
len = recvmsg(fd, msg, flags);
} while (len < 0 && (errno == EINTR || errno == EAGAIN));
if (len < 0) {
fprintf(stderr, "netlink receive error %s (%d)\n",
strerror(errno), errno);
return -errno;
}
if (len == 0) {
fprintf(stderr, "EOF on netlink\n");
return -ENODATA;
}
return len;
}
```
https:/
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308553353)
nit: should also error on `recv_result == 0`, e.g. from libnetlink:
```c
static int __rtnl_recvmsg(int fd, struct msghdr *msg, int flags)
{
int len;
do {
len = recvmsg(fd, msg, flags);
} while (len < 0 && (errno == EINTR || errno == EAGAIN));
if (len < 0) {
fprintf(stderr, "netlink receive error %s (%d)\n",
strerror(errno), errno);
return -errno;
}
if (len == 0) {
fprintf(stderr, "EOF on netlink\n");
return -ENODATA;
}
return len;
}
```
https:/
...
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308578286)
do we want to test that in `feature_assumeutxo.py` or in `rpc_blockchain.py`
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308578286)
do we want to test that in `feature_assumeutxo.py` or in `rpc_blockchain.py`
💬 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_r2308638969)
For context on this, IPv6 allows "scoped addresses", e.g. I can send a packet to a non-globally unique address, like a link-local address that has two destinations on two different links, so in order to disambiguate scoped addresses [RFC4007#section-6](https://datatracker.ietf.org/doc/html/rfc4007#section-6) recommends the use of unique zone indexes:
> Because the same non-global address may be in use in more than one zone of the same scope (e.g., the use of link-local address fe80::1 in tw
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308638969)
For context on this, IPv6 allows "scoped addresses", e.g. I can send a packet to a non-globally unique address, like a link-local address that has two destinations on two different links, so in order to disambiguate scoped addresses [RFC4007#section-6](https://datatracker.ietf.org/doc/html/rfc4007#section-6) recommends the use of unique zone indexes:
> Because the same non-global address may be in use in more than one zone of the same scope (e.g., the use of link-local address fe80::1 in tw
...
💬 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_r2308655496)
nit:
`processed_one`, `multi_part` and the check at the bottom of the outer for loop could be dropped, with the following check:
```suggestion
if (!(hdr->nlmsg_flags & NLM_F_MULTI)) {
done = true;
}
```
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308655496)
nit:
`processed_one`, `multi_part` and the check at the bottom of the outer for loop could be dropped, with the following check:
```suggestion
if (!(hdr->nlmsg_flags & NLM_F_MULTI)) {
done = true;
}
```
💬 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_r2308665037)
nit:
I think we should return early on `total_bytes_read == 0` as well.
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308665037)
nit:
I think we should return early on `total_bytes_read == 0` as well.
💬 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#issuecomment-3235124766)
untested crACK 4c531782569b42fc474
https://github.com/bitcoin/bitcoin/pull/32159/commits/57ce645f05d18d8ad10711c347a5989076f1f788 corrects a bug where `QueryDefaultGatewayImpl` would return *not* default gateways.
https://github.com/bitcoin/bitcoin/pull/32159/commits/42e99ad77396e4e9b02d67daf46349e215e99a0f adds a belt and suspenders check that we have received a sensible reply to our `GETROUTE` message.
https://github.com/bitcoin/bitcoin/pull/32159/commits/4c531782569b42fc47478a468f4a7
...
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3235124766)
untested crACK 4c531782569b42fc474
https://github.com/bitcoin/bitcoin/pull/32159/commits/57ce645f05d18d8ad10711c347a5989076f1f788 corrects a bug where `QueryDefaultGatewayImpl` would return *not* default gateways.
https://github.com/bitcoin/bitcoin/pull/32159/commits/42e99ad77396e4e9b02d67daf46349e215e99a0f adds a belt and suspenders check that we have received a sensible reply to our `GETROUTE` message.
https://github.com/bitcoin/bitcoin/pull/32159/commits/4c531782569b42fc47478a468f4a7
...
📝 achow101 opened a pull request: "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet"
(https://github.com/bitcoin/bitcoin/pull/33268)
One of the ways that the wallet would determine if a transaction belonged to the wallet was by checking if the total amount being spent by a transaction from outputs known to the wallet was greater than 0. This has worked fine until recently since there was no reason for 0-value outputs to be created. However, with ephemeral dust and P2A, it is possible to create standard 0-value outputs, and the wallet was not correctly identifying the spends of such outputs. This PR updates `IsFromMe` to only
...
(https://github.com/bitcoin/bitcoin/pull/33268)
One of the ways that the wallet would determine if a transaction belonged to the wallet was by checking if the total amount being spent by a transaction from outputs known to the wallet was greater than 0. This has worked fine until recently since there was no reason for 0-value outputs to be created. However, with ephemeral dust and P2A, it is possible to create standard 0-value outputs, and the wallet was not correctly identifying the spends of such outputs. This PR updates `IsFromMe` to only
...
💬 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_r2308685773)
Feel free to mark resolved, this suggestion is actually incorrect, since when `recv_result == 0` the `NLMSG_OK` check [below](https://github.com/bitcoin/bitcoin/blob/4c531782569b42fc47478a468f4a79e0e2d93946/src/common/netif.cpp#L110) would fail:
```c
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len <= (len))
```
https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c
...
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308685773)
Feel free to mark resolved, this suggestion is actually incorrect, since when `recv_result == 0` the `NLMSG_OK` check [below](https://github.com/bitcoin/bitcoin/blob/4c531782569b42fc47478a468f4a79e0e2d93946/src/common/netif.cpp#L110) would fail:
```c
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
(nlh)->nlmsg_len <= (len))
```
https://github.com/torvalds/linux/blob/39f90c1967215375f7d87b81d14b0f3ed6b40c
...
🚀 achow101 merged a pull request: "Revert compact block cache inefficiencies"
(https://github.com/bitcoin/bitcoin/pull/33253)
(https://github.com/bitcoin/bitcoin/pull/33253)
💬 achow101 commented on pull request "rpc: require integer verbosity; remove boolean 'verbose'":
(https://github.com/bitcoin/bitcoin/pull/33214#issuecomment-3235256712)
The way to deprecate these properly would be to first require `-deprecatedrpc=boolverbose` (or similar) which gates the old behavior. Then the following release can remove them.
(https://github.com/bitcoin/bitcoin/pull/33214#issuecomment-3235256712)
The way to deprecate these properly would be to first require `-deprecatedrpc=boolverbose` (or similar) which gates the old behavior. Then the following release can remove them.
💬 davidgumberg commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2308783181)
In commit https://github.com/bitcoin/bitcoin/pull/30469/commits/e7b111a63067cf6c66140228c28ab1fa96f7ff18 (_refactor, index: Remove member variables in coinstatsindex_):
for other reviewers: `read_out.second.total_subidy` is updated below:
https://github.com/bitcoin/bitcoin/blob/e7b111a63067cf6c66140228c28ab1fa96f7ff18/src/index/coinstatsindex.cpp#L200
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2308783181)
In commit https://github.com/bitcoin/bitcoin/pull/30469/commits/e7b111a63067cf6c66140228c28ab1fa96f7ff18 (_refactor, index: Remove member variables in coinstatsindex_):
for other reviewers: `read_out.second.total_subidy` is updated below:
https://github.com/bitcoin/bitcoin/blob/e7b111a63067cf6c66140228c28ab1fa96f7ff18/src/index/coinstatsindex.cpp#L200