💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279057067)
The IWYU commit? The rationale was that you asked me above to make the change, which is the only reason I included it in this PR.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279057067)
The IWYU commit? The rationale was that you asked me above to make the change, which is the only reason I included it in this PR.
🚀 fanquake merged a pull request: "qa, doc: Fix comment"
(https://github.com/bitcoin/bitcoin/pull/28181)
(https://github.com/bitcoin/bitcoin/pull/28181)
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279066035)
I think I only suggested to move the export: https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277709470 (not *remove* it).
On a second thought, I am wondering if it makes more sense to do something like https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279032950 instead.
I mean anything that is correct is fine here, but in light of multiple alternatives, it seems odd to pick one, then revert it, and pick something else? (Sorry for coming up with the second alternative)
...
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279066035)
I think I only suggested to move the export: https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277709470 (not *remove* it).
On a second thought, I am wondering if it makes more sense to do something like https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279032950 instead.
I mean anything that is correct is fine here, but in light of multiple alternatives, it seems odd to pick one, then revert it, and pick something else? (Sorry for coming up with the second alternative)
...
💬 vasild commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279068859)
Is it not possible to replace all of that with just:
```cpp
class Txid : public uint256
{
};
class Wtxid : public uint256
{
};
```
It achieves the purpose of not allowing conversion and comparison between `Txid` and `Wtxid` and each one of them can be treated as `uint256`.
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279068859)
Is it not possible to replace all of that with just:
```cpp
class Txid : public uint256
{
};
class Wtxid : public uint256
{
};
```
It achieves the purpose of not allowing conversion and comparison between `Txid` and `Wtxid` and each one of them can be treated as `uint256`.
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279069995)
Ok. Going to implement https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279032950 and fix all issues.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279069995)
Ok. Going to implement https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279032950 and fix all issues.
💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658055880)
> Depends still takes more than an hour for me
Yes, there seems to be quite an overhead in calling qemu for each configure check, and each build file. This is really the cost that is paid in this pull, but I think it is worth it the benefits.
> Am I correct that this build will use a depends cache on actual CI though, which should speed it up there significantly?
CI will run on `aarch64` metal, so qemu will never be called. (depends is also cached, but this is happening regardless).
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658055880)
> Depends still takes more than an hour for me
Yes, there seems to be quite an overhead in calling qemu for each configure check, and each build file. This is really the cost that is paid in this pull, but I think it is worth it the benefits.
> Am I correct that this build will use a depends cache on actual CI though, which should speed it up there significantly?
CI will run on `aarch64` metal, so qemu will never be called. (depends is also cached, but this is happening regardless).
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1279081462)
CJDNS vs IPv6 difference may not be very obvious to some people.
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1279081462)
CJDNS vs IPv6 difference may not be very obvious to some people.
💬 Daniel600 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658076861)
This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
At GAP600 we continue to see strong use of our service for BTC we have seen circa 350k unique trx hash per month (over the last 3 months) requested to our platform. Our clients include - Coinpayments, Coi
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658076861)
This would unnecessarily and extremely negatively impact merchants and users who choose to accept 0-conf while using mitigation tools like GAP600. This negative impact could be avoided by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
At GAP600 we continue to see strong use of our service for BTC we have seen circa 350k unique trx hash per month (over the last 3 months) requested to our platform. Our clients include - Coinpayments, Coi
...
💬 fanquake commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1658077549)
> Can you add a link to documentation on how to set that up?
Trying to find that at the moment. Maybe it doesn't exist.
> unless you can point to a bug that was found with arm64, but not with amd64.
I think it's at least worth bringing up, as the PR as presented, didn't mention that it drops (direct) testing of one of our release platforms entirely, and presented this change as-if just swapping underlying CI provider.
In terms of differences, there are code paths that are currently e
...
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1658077549)
> Can you add a link to documentation on how to set that up?
Trying to find that at the moment. Maybe it doesn't exist.
> unless you can point to a bug that was found with arm64, but not with amd64.
I think it's at least worth bringing up, as the PR as presented, didn't mention that it drops (direct) testing of one of our release platforms entirely, and presented this change as-if just swapping underlying CI provider.
In terms of differences, there are code paths that are currently e
...
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1658080945)
I would be happy to re-ack if followups are incorporated here. That would also keep git history cleaner. Can't amend commit messages in a followup, it will remain as "IPv4 and IPv6 as one network" forever in git history, could confuse the future generations (or me after a few months have passed and I have forgotten everything :disappointed:).
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1658080945)
I would be happy to re-ack if followups are incorporated here. That would also keep git history cleaner. Can't amend commit messages in a followup, it will remain as "IPv4 and IPv6 as one network" forever in git history, could confuse the future generations (or me after a few months have passed and I have forgotten everything :disappointed:).
💬 fanquake commented on pull request "ci: Use documented `CCACHE_MAXSIZE` instead of `CCACHE_SIZE`":
(https://github.com/bitcoin/bitcoin/pull/28188#issuecomment-1658082009)
Looks like this is still working as intended, and the cache sizes are being set, i.e 250mb for the msan job https://cirrus-ci.com/task/5055152978132992?logs=ci#L1997:
```bash
Cache size (GB): 0.22 / 0.25 (86.73 %)
```
(https://github.com/bitcoin/bitcoin/pull/28188#issuecomment-1658082009)
Looks like this is still working as intended, and the cache sizes are being set, i.e 250mb for the msan job https://cirrus-ci.com/task/5055152978132992?logs=ci#L1997:
```bash
Cache size (GB): 0.22 / 0.25 (86.73 %)
```
🚀 fanquake merged a pull request: "ci: Use documented `CCACHE_MAXSIZE` instead of `CCACHE_SIZE`"
(https://github.com/bitcoin/bitcoin/pull/28188)
(https://github.com/bitcoin/bitcoin/pull/28188)
💬 Sjors commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1658088944)
> puts the burden of proof on arguing that we should have an arbitrary 80 byte limit.
Perhaps. But that's not an argument to catch them by surprise.
> Any difference in relay policy can do that.
I'm not sure if that's true. The introduction of new soft forks like Taproot do allow this to happen, but that's not a minor tweak and it came with significant benefit.
In any case you can just state the above on the mailinglist and see if anyone disagrees. Other proposed policy tweaks, lik
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1658088944)
> puts the burden of proof on arguing that we should have an arbitrary 80 byte limit.
Perhaps. But that's not an argument to catch them by surprise.
> Any difference in relay policy can do that.
I'm not sure if that's true. The introduction of new soft forks like Taproot do allow this to happen, but that's not a minor tweak and it came with significant benefit.
In any case you can just state the above on the mailinglist and see if anyone disagrees. Other proposed policy tweaks, lik
...
💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1658089946)
yeah, it should be a trivial two line diff to switch to arm, once and if GitHub supports it. Seems fine to use intel for now, if the alternative is no macOS CI at all.
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1658089946)
yeah, it should be a trivial two line diff to switch to arm, once and if GitHub supports it. Seems fine to use intel for now, if the alternative is no macOS CI at all.
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1279106923)
> Having a map where a `std::array` would be fine is kind of lame
Why? Semantically the closest container to what is needed here is a map. Do you mean from performance point of view? I would go with map first and consider an optimization only if it has been demonstrated that the optimization results in faster code or smaller memory footprint.
To be explicit - I would also ack this PR if this is changed to array. I think the map vs array discussion is minor for this PR and shouldn't block i
...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1279106923)
> Having a map where a `std::array` would be fine is kind of lame
Why? Semantically the closest container to what is needed here is a map. Do you mean from performance point of view? I would go with map first and consider an optimization only if it has been demonstrated that the optimization results in faster code or smaller memory footprint.
To be explicit - I would also ack this PR if this is changed to array. I think the map vs array discussion is minor for this PR and shouldn't block i
...
💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279109114)
That is what I originally had in this PR but it actually doesn't achieve the same on all fronts, so I decided to make it explicit to avoid subtle bugs.
e.g. the following was still permitted:
```c++
Txid txid;
Wtxid wtxid{txid};
// or
wtxid.Compare(txid);
// or
bool equal = (wtxid == txid); // same for !=
```
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279109114)
That is what I originally had in this PR but it actually doesn't achieve the same on all fronts, so I decided to make it explicit to avoid subtle bugs.
e.g. the following was still permitted:
```c++
Txid txid;
Wtxid wtxid{txid};
// or
wtxid.Compare(txid);
// or
bool equal = (wtxid == txid); // same for !=
```
💬 dergoegge commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1658115391)
Concept ACK
> Merging the two means a future V2Transport can handle all this interaction without callers needing to be aware.
🚀
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1658115391)
Concept ACK
> Merging the two means a future V2Transport can handle all this interaction without callers needing to be aware.
🚀
✅ fanquake closed an issue: "fuzz: connman, `m_nodes` is always empty"
(https://github.com/bitcoin/bitcoin/issues/27980)
(https://github.com/bitcoin/bitcoin/issues/27980)
🚀 fanquake merged a pull request: "fuzz: use `ConnmanTestMsg` in `connman`"
(https://github.com/bitcoin/bitcoin/pull/28091)
(https://github.com/bitcoin/bitcoin/pull/28091)
💬 Sjors commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658120425)
> by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
Including the original outputs is not part of BIP 125. It's not realistic to expect an entirely new complex RBF proposal, especially one that's not incentive compatible for miners. It's also something for the mailinglist.
> We have not seen any impact of full RBF on double spend rates for our trxs
This does seem relevant. But it can be interpreted both ways: it could also mean y
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658120425)
> by simply adding first seen safe rule - ie a trx can be replaced but needs to include the original outputs.
Including the original outputs is not part of BIP 125. It's not realistic to expect an entirely new complex RBF proposal, especially one that's not incentive compatible for miners. It's also something for the mailinglist.
> We have not seen any impact of full RBF on double spend rates for our trxs
This does seem relevant. But it can be interpreted both ways: it could also mean y
...