💬 jonatack commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2388680568)
> The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing it now.
@Mackain The commit message wasn't updated yet, could you update it and re-push the same commit (thanks!)
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2388680568)
> The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing it now.
@Mackain The commit message wasn't updated yet, could you update it and re-push the same commit (thanks!)
💬 maflcko commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2388688159)
Please report the CI failure. Otherwise, it will be harder to track and fix it. This is explained in the log:
```
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2388688159)
Please report the CI failure. Otherwise, it will be harder to track and fix it. This is explained in the log:
```
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
🤔 hodlinator reviewed a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2342962102)
stickies-v:
> All format strings are known at compile-time, so any formatting errors are programming errors. Handling that with `util::Result` would be both a cumbersome interface and bad practice imo.
This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.
maflcko:
> I agree that `util::Result` d
...
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2342962102)
stickies-v:
> All format strings are known at compile-time, so any formatting errors are programming errors. Handling that with `util::Result` would be both a cumbersome interface and bad practice imo.
This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.
maflcko:
> I agree that `util::Result` d
...
💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784535464)
Changing tinyformat to support `string_view` is definitely out of scope for this PR.
I would at least want to make it clearer that format strings are separate from other parameters, like this or with other words to that effect:
```suggestion
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```
Even though I decided
...
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784535464)
Changing tinyformat to support `string_view` is definitely out of scope for this PR.
I would at least want to make it clearer that format strings are separate from other parameters, like this or with other words to that effect:
```suggestion
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```
Even though I decided
...
💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#issuecomment-2388722857)
@maflcko
> Are you still working on this?
I'm waiting on #30928 before un-drafting this.
(https://github.com/bitcoin/bitcoin/pull/30933#issuecomment-2388722857)
@maflcko
> Are you still working on this?
I'm waiting on #30928 before un-drafting this.
💬 maflcko commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2388728933)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2388728933)
Concept ACK
💬 l0rinc commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784608370)
It seems to me we're still hardcoding the major version - can't we change this during migration instead?
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784608370)
It seems to me we're still hardcoding the major version - can't we change this during migration instead?
💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388768319)
> This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.
If translations are malformed to induce a tinyformat error where one wouldn't be present in the non-translated string, I don't think the solution would to use `Result`. This seems no different from an otherwise fully tinyformat-valid string that
...
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388768319)
> This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.
If translations are malformed to induce a tinyformat error where one wouldn't be present in the non-translated string, I don't think the solution would to use `Result`. This seems no different from an otherwise fully tinyformat-valid string that
...
💬 hebasto commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784617293)
> It seems to me we're still hardcoding the major version
I meant the variable names. The full minimum required version was not supposed to change.
> can't we change this during migration instead?
It is done in https://github.com/bitcoin/bitcoin/pull/30997. This PR split from it to make the https://github.com/bitcoin/bitcoin/pull/30997 easier to review.
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784617293)
> It seems to me we're still hardcoding the major version
I meant the variable names. The full minimum required version was not supposed to change.
> can't we change this during migration instead?
It is done in https://github.com/bitcoin/bitcoin/pull/30997. This PR split from it to make the https://github.com/bitcoin/bitcoin/pull/30997 easier to review.
💬 vasild commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388772718)
> increase the cost of detecting that they run a bitcoin node
How? Given that the spy does not need to inspect the traffic at all:
> My node will connect to publicly known bitcoin nodes and the mere fact that I have a TCP connection to an addr:port, where it is known that a bitcoin node is running, is already revealing enough.
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388772718)
> increase the cost of detecting that they run a bitcoin node
How? Given that the spy does not need to inspect the traffic at all:
> My node will connect to publicly known bitcoin nodes and the mere fact that I have a TCP connection to an addr:port, where it is known that a bitcoin node is running, is already revealing enough.
💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2388779305)
Thanks for the feedback!
>I wonder if it's not easier to minimize what is being caught in the IOError try block and just remove missing bins from the BINARIES list? This list is used later on in the script too so in my opinion that is probably the better option. Otherwise we will end up referencing non-existing binaries/manpages in the existing manpages.
I agree and have updated the commit to reflect your suggestions. I just needed to make a copy of BINARIES so that it's possible to remove
...
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2388779305)
Thanks for the feedback!
>I wonder if it's not easier to minimize what is being caught in the IOError try block and just remove missing bins from the BINARIES list? This list is used later on in the script too so in my opinion that is probably the better option. Otherwise we will end up referencing non-existing binaries/manpages in the existing manpages.
I agree and have updated the commit to reflect your suggestions. I just needed to make a copy of BINARIES so that it's possible to remove
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388783616)
@fanquake
> Building [291ec5e](https://github.com/bitcoin/bitcoin/commit/291ec5e2a876622a051baf6e33a4d4d592e6568e) (on arm64):
Thanks! I explicitly disabled unused deployment tools and a few other features for the `native_qt` package.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388783616)
@fanquake
> Building [291ec5e](https://github.com/bitcoin/bitcoin/commit/291ec5e2a876622a051baf6e33a4d4d592e6568e) (on arm64):
Thanks! I explicitly disabled unused deployment tools and a few other features for the `native_qt` package.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388792957)
> @hebasto now I can't build QT with depends anymore ([291ec5e](https://github.com/bitcoin/bitcoin/commit/291ec5e2a876622a051baf6e33a4d4d592e6568e)): https://gist.github.com/Sjors/e42242b27b71c98ee10d60d58a20872a
>
> (since the last time I tested I uninstalled Xcode as part of debugging #30978)
Based on the Qt source code, it appears that the build configuration may depend on Xcode. Could you please confirm whether reinstalling Xcode resolves the issue for you? If so, I will need to apply
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388792957)
> @hebasto now I can't build QT with depends anymore ([291ec5e](https://github.com/bitcoin/bitcoin/commit/291ec5e2a876622a051baf6e33a4d4d592e6568e)): https://gist.github.com/Sjors/e42242b27b71c98ee10d60d58a20872a
>
> (since the last time I tested I uninstalled Xcode as part of debugging #30978)
Based on the Qt source code, it appears that the build configuration may depend on Xcode. Could you please confirm whether reinstalling Xcode resolves the issue for you? If so, I will need to apply
...
💬 sipa commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388794160)
@vasild That assumes the attacker has sufficient monitoring infrastructure in place to maintain an accurate list of Bitcoin P2P ip:ports. That's obviously not impossible, but it is a significantly larger effort than stateless packet inspection looking for the string "\xf9\xbe\xb4\xd99version\x00\x00\x00\x00\x00".
And yes, very little in BIP324 protects against an attacker deliberately targetting specific individuals - its goal is increasing costs for large-scale monitoring. From that perspect
...
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388794160)
@vasild That assumes the attacker has sufficient monitoring infrastructure in place to maintain an accurate list of Bitcoin P2P ip:ports. That's obviously not impossible, but it is a significantly larger effort than stateless packet inspection looking for the string "\xf9\xbe\xb4\xd99version\x00\x00\x00\x00\x00".
And yes, very little in BIP324 protects against an attacker deliberately targetting specific individuals - its goal is increasing costs for large-scale monitoring. From that perspect
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388854676)
> Thanks! I explicitly disabled unused deployment tools and a few other features for the native_qt package.
I tried building the new branch (a91d53615a7bb1da7ccb59e7876a1e31c94013b7) and got:
```bash
make -C depends HOST=x86_64-apple-darwin qt
<snip>
-- [QtBase] Searching for tool 'Qt6::qvkgen' in package Qt6GuiTools.
-- [QtBase] Qt6::qvkgen was found at /bitcoin/depends/x86_64-apple-darwin/native/./libexec/qvkgen using package Qt6GuiTools.
-- [QtBase] Tool 'Qt6::qtpaths' was found at /
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388854676)
> Thanks! I explicitly disabled unused deployment tools and a few other features for the native_qt package.
I tried building the new branch (a91d53615a7bb1da7ccb59e7876a1e31c94013b7) and got:
```bash
make -C depends HOST=x86_64-apple-darwin qt
<snip>
-- [QtBase] Searching for tool 'Qt6::qvkgen' in package Qt6GuiTools.
-- [QtBase] Qt6::qvkgen was found at /bitcoin/depends/x86_64-apple-darwin/native/./libexec/qvkgen using package Qt6GuiTools.
-- [QtBase] Tool 'Qt6::qtpaths' was found at /
...
💬 jonatack commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388870050)
> However, as I pointed out in the linked issue, I am somewhat concerned about a potential future where it becomes harder for other/older software to connect to the network if large swaths move to be v2-only. Because of that, I wonder if it isn't better to make this only apply to outbound connections, and advise people who want more private operation to not listen for inbound connections.
Yes. I may be potentially concept ack here if this proposal adopts the suggestions in https://github.com/
...
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388870050)
> However, as I pointed out in the linked issue, I am somewhat concerned about a potential future where it becomes harder for other/older software to connect to the network if large swaths move to be v2-only. Because of that, I wonder if it isn't better to make this only apply to outbound connections, and advise people who want more private operation to not listen for inbound connections.
Yes. I may be potentially concept ack here if this proposal adopts the suggestions in https://github.com/
...
💬 jonatack commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388883991)
> However, as I pointed out in the linked issue, I am somewhat concerned about a potential future where it becomes harder for other/older software to connect to the network if large swaths move to be v2-only. Because of that, I wonder if it isn't better to make this only apply to outbound connections, and advise people who want more private operation to not listen for inbound connections.
Yes. I may be potentially concept/approach ack here if this proposal adopts the suggestion above / in htt
...
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388883991)
> However, as I pointed out in the linked issue, I am somewhat concerned about a potential future where it becomes harder for other/older software to connect to the network if large swaths move to be v2-only. Because of that, I wonder if it isn't better to make this only apply to outbound connections, and advise people who want more private operation to not listen for inbound connections.
Yes. I may be potentially concept/approach ack here if this proposal adopts the suggestion above / in htt
...
💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388899694)
> This is a good argument except we do have some strings only known at runtime, notably translations
I chose my wording "known at compile-time" carefully for exactly this reason: they're _known_ at compile-time, we just choose not to implement the (complete) validation logic because it much more complex than just returning an error string. It's not worth arguing semantics too much, but in my view that's very much still a programming error and not something that callsites should be aware of, l
...
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388899694)
> This is a good argument except we do have some strings only known at runtime, notably translations
I chose my wording "known at compile-time" carefully for exactly this reason: they're _known_ at compile-time, we just choose not to implement the (complete) validation logic because it much more complex than just returning an error string. It's not worth arguing semantics too much, but in my view that's very much still a programming error and not something that callsites should be aware of, l
...
💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784734716)
> I would at least want to make it clearer that format strings are separate from other parameters
I agree with that, and I'll incorporate your first suggestion (I [don't agree](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388899694) with the translations bit) if I have to force-push, thanks!
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784734716)
> I would at least want to make it clearer that format strings are separate from other parameters
I agree with that, and I'll incorporate your first suggestion (I [don't agree](https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388899694) with the translations bit) if I have to force-push, thanks!
✅ mzumsande closed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114)
(https://github.com/bitcoin/bitcoin/pull/26114)