💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658020560)
> What would you like to see adjusted?
I think the link to the external repo can be dropped, given that the source code lives in this repo, and linking to an outside one could increase confusion as to which place is the correct one to submit patches to?
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658020560)
> What would you like to see adjusted?
I think the link to the external repo can be dropped, given that the source code lives in this repo, and linking to an outside one could increase confusion as to which place is the correct one to submit patches to?
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279050723)
Then maybe drop the commit. Otherwise it seems odd to touch the same line of code twice, where the first one doesn't really have a rationale?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279050723)
Then maybe drop the commit. Otherwise it seems odd to touch the same line of code twice, where the first one doesn't really have a rationale?
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658024064)
> I think the link to the external repo can be dropped,
Ok.
Also fixed the lint regex.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658024064)
> I think the link to the external repo can be dropped,
Ok.
Also fixed the lint regex.
💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658031353)
@MarcoFalke
> Yes, this is expected. Instead of just running `bitcoind` through qemu, now the whole container is run through qemu.
Ok running this again... to setup and build Depends still takes more than an hour for me (with it taking 56 mins to progress past `building qt`) which seems relatively annoying. Am I correct that this build will use a depends cache on actual CI though, which should speed it up there significantly?
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658031353)
@MarcoFalke
> Yes, this is expected. Instead of just running `bitcoind` through qemu, now the whole container is run through qemu.
Ok running this again... to setup and build Depends still takes more than an hour for me (with it taking 56 mins to progress past `building qt`) which seems relatively annoying. Am I correct that this build will use a depends cache on actual CI though, which should speed it up there significantly?
💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1658035143)
@pinheadmz yeah this is probably the best attempt at minimizing the problem I mentioned. Whether it's satisfactory is still at question :) I'm looking forward to seeing what others think.
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1658035143)
@pinheadmz yeah this is probably the best attempt at minimizing the problem I mentioned. Whether it's satisfactory is still at question :) I'm looking forward to seeing what others think.
💬 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 !=
```