💬 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
...
💬 Sjors commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1658134400)
That test is a good starting point. The current version picks one random number so it's not really testing the limit. It also doesn't test N-of-N (hopefully that's not too slow).
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-1658134400)
That test is a good starting point. The current version picks one random number so it's not really testing the limit. It also doesn't test N-of-N (hopefully that's not too slow).
💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658137282)
Unfortunately that build broke again for me for another seemingly unrealted reason. I have the correct version of qemu this time for sure:
```log
will@ubuntu in ~
$ qemu-s390x --version
qemu-s390x version 7.2.0 (Debian 1:7.2+dfsg-5ubuntu2.2)
Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers
```
I don't think I can test this on my side, but I am a concept ACK on these changes, and they _look_ correct to me.
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658137282)
Unfortunately that build broke again for me for another seemingly unrealted reason. I have the correct version of qemu this time for sure:
```log
will@ubuntu in ~
$ qemu-s390x --version
qemu-s390x version 7.2.0 (Debian 1:7.2+dfsg-5ubuntu2.2)
Copyright (c) 2003-2022 Fabrice Bellard and the QEMU Project developers
```
I don't think I can test this on my side, but I am a concept ACK on these changes, and they _look_ correct to me.
💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658145208)
Feel free to create an issue with the unrelated stuff you ran into. Happy to take a look. Personally, I spin up a fresh VM (in a cloud or locally) to do testing, so I likely miss most CI UX issues.
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658145208)
Feel free to create an issue with the unrelated stuff you ran into. Happy to take a look. Personally, I spin up a fresh VM (in a cloud or locally) to do testing, so I likely miss most CI UX issues.
💬 vasild 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-1658150572)
> this might better wait for authenticated connections
I2P and CJDNS connections are already authenticated. Addresses in those networks represent the node's public key and connections have a source address. Thus to spoof, one would need to steal the private key from its owner.
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1658150572)
> this might better wait for authenticated connections
I2P and CJDNS connections are already authenticated. Addresses in those networks represent the node's public key and connections have a source address. Thus to spoof, one would need to steal the private key from its owner.
💬 Daniel600 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658170208)
> > 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 your customers have no intention to double-spend you even though it became technically easier.
>
> Opportunity makes a thief, but this default won't change the opportunity by much. A thief needs software that will perform a double-spend without the RBF flag. Anyone making such software will know they need to either use [prefer
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658170208)
> > 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 your customers have no intention to double-spend you even though it became technically easier.
>
> Opportunity makes a thief, but this default won't change the opportunity by much. A thief needs software that will perform a double-spend without the RBF flag. Anyone making such software will know they need to either use [prefer
...
💬 Sjors commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658191272)
> with FullRBF significantly adopted by miners
If @petertodd's analysis above is correct, that's already the case and this PR won't change that.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658191272)
> with FullRBF significantly adopted by miners
If @petertodd's analysis above is correct, that's already the case and this PR won't change that.
💬 Sjors commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1658216350)
Even assuming pinning was an issue, it wouldn't be a blocker. It could simply be released at the same time as some other policy change (package relay, #27677, v3 transactions, the next soft fork, etc). Or even just with sufficient heads-up.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1658216350)
Even assuming pinning was an issue, it wouldn't be a blocker. It could simply be released at the same time as some other policy change (package relay, #27677, v3 transactions, the next soft fork, etc). Or even just with sufficient heads-up.
📝 MarcoFalke opened a pull request: "refactor: Remove unused MessageStartChars parameters from BlockManager methods"
(https://github.com/bitcoin/bitcoin/pull/28191)
Seems odd to expose these for mocking, when it is not needed.
Fix this by removing the the unused parameters and use the already existing member field instead.
(https://github.com/bitcoin/bitcoin/pull/28191)
Seems odd to expose these for mocking, when it is not needed.
Fix this by removing the the unused parameters and use the already existing member field instead.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279249579)
_(beware, https://bikeshed.com/)_
I think any constant is ok and `bitcoin-submittx`'s string (`/pynode:0.0.1/`) was already suggested by @sipa here https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1517896418. That suggestion has 6 positive reactions. It is about increasing the anonymity set, in case different versions or different softwares use this technique.
> some kind of standard for "I am not telling you"
I don't think there is, but we can standardize `/pynode:0.0.1/` as
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279249579)
_(beware, https://bikeshed.com/)_
I think any constant is ok and `bitcoin-submittx`'s string (`/pynode:0.0.1/`) was already suggested by @sipa here https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1517896418. That suggestion has 6 positive reactions. It is about increasing the anonymity set, in case different versions or different softwares use this technique.
> some kind of standard for "I am not telling you"
I don't think there is, but we can standardize `/pynode:0.0.1/` as
...
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1658332910)
`7a3206dc8e...2541f09439`: add a comment wrt https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276240357
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1658332910)
`7a3206dc8e...2541f09439`: add a comment wrt https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276240357