🤔 glozow reviewed a pull request: "test: Fix multiprocess CI timeout in p2p_tx_download"
(https://github.com/bitcoin/bitcoin/pull/29926#pullrequestreview-2014393208)
(cc @theStack, indeed looks like it's from #29827)
(https://github.com/bitcoin/bitcoin/pull/29926#pullrequestreview-2014393208)
(cc @theStack, indeed looks like it's from #29827)
💬 glozow commented on pull request "test: Fix multiprocess CI timeout in p2p_tx_download":
(https://github.com/bitcoin/bitcoin/pull/29926#discussion_r1574531190)
I'm not convinced that the +300 would make a difference since the timing for getdata isn't variable like announcements are (which was the case in #28321)
(https://github.com/bitcoin/bitcoin/pull/29926#discussion_r1574531190)
I'm not convinced that the +300 would make a difference since the timing for getdata isn't variable like announcements are (which was the case in #28321)
💬 glozow commented on pull request "test: Don't fetch orphan parent when parent is in orphanage":
(https://github.com/bitcoin/bitcoin/pull/29915#discussion_r1574537327)
`wait_for_parent_requests` already checks this, since it requires that the list of getdatas match exactly (nothing additional allowed)?
(https://github.com/bitcoin/bitcoin/pull/29915#discussion_r1574537327)
`wait_for_parent_requests` already checks this, since it requires that the list of getdatas match exactly (nothing additional allowed)?
💬 Sjors commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2069067883)
I was going to suggest changing `doc/dependencies.md` to clarify the minimum clang version is 14.0.3 rather than 14.0, but #29165 bumps it to 15.0 anyway.
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2069067883)
I was going to suggest changing `doc/dependencies.md` to clarify the minimum clang version is 14.0.3 rather than 14.0, but #29165 bumps it to 15.0 anyway.
💬 hebasto commented on issue "CMake-based build system tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-2069074476)
@Sjors
> What's next?
>
> It would be useful to have a link to build instructions too, not just the cheat cheat (or just add the standard `mkdir build`, `cmake ..` incantations to the cheat cheat).
>
> In noticed that in your `hebasto/cmake-staging` branch the macOS instructions still assume autotools.
WIP :)
Your testing and reviewing https://github.com/hebasto/bitcoin/pull/163 will be much appreciated!
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-2069074476)
@Sjors
> What's next?
>
> It would be useful to have a link to build instructions too, not just the cheat cheat (or just add the standard `mkdir build`, `cmake ..` incantations to the cheat cheat).
>
> In noticed that in your `hebasto/cmake-staging` branch the macOS instructions still assume autotools.
WIP :)
Your testing and reviewing https://github.com/hebasto/bitcoin/pull/163 will be much appreciated!
💬 maflcko commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2069098524)
> I was going to suggest changing `doc/dependencies.md` to clarify the minimum clang version is 14.0.3 rather than 14.0, but #29165 bumps it to 15.0 anyway.
That would not be accurate. Clang 14 is indeed supported, see also https://godbolt.org/z/61seKv74b
The problem here is that Apple Clang 14.0.0 is apparently not based on llvm 14.0.0. I don't have further insights into Apples versioning nonsense, so I can't help here.
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2069098524)
> I was going to suggest changing `doc/dependencies.md` to clarify the minimum clang version is 14.0.3 rather than 14.0, but #29165 bumps it to 15.0 anyway.
That would not be accurate. Clang 14 is indeed supported, see also https://godbolt.org/z/61seKv74b
The problem here is that Apple Clang 14.0.0 is apparently not based on llvm 14.0.0. I don't have further insights into Apples versioning nonsense, so I can't help here.
💬 Sjors commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2069100533)
Can you add a note to `doc/build-osx.md` that users of macOS < 13 need to install `clang` via homebrew?
My Intel iMac from 2017 runs macOS 13.6.6 and has clang 15.0.0. It's installed at `/usr/bin/clang` so not Homebrew.
Xcode 14.3.1 and newer ship with clang 15.0:
https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)
The system requirements for that is macOS Ventura 13.0 or later: https://developer.apple.com/documentation/xcode-release-notes/xcode-14_3_1-relea
...
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2069100533)
Can you add a note to `doc/build-osx.md` that users of macOS < 13 need to install `clang` via homebrew?
My Intel iMac from 2017 runs macOS 13.6.6 and has clang 15.0.0. It's installed at `/usr/bin/clang` so not Homebrew.
Xcode 14.3.1 and newer ship with clang 15.0:
https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)
The system requirements for that is macOS Ventura 13.0 or later: https://developer.apple.com/documentation/xcode-release-notes/xcode-14_3_1-relea
...
💬 Sjors commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2069107675)
I see, so Apple clang needs to be >=14.0.3, but for other operating systems 14.0 is fine.
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2069107675)
I see, so Apple clang needs to be >=14.0.3, but for other operating systems 14.0 is fine.
💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2069108058)
Ok, I see. Then I'll be able to get to this a bit faster.
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2069108058)
Ok, I see. Then I'll be able to get to this a bit faster.
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2069109399)
> Can you add a note to `doc/build-osx.md` that users of macOS < 13 need to install `clang` via homebrew?
#https://github.com/bitcoin/bitcoin/issues/29918 is an unrelated issue existing on current master, so I think a separate pull request would be more appropriate. Also, I don't have or use macOS/Apple, so someone else creating that pull request would be beneficial.
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2069109399)
> Can you add a note to `doc/build-osx.md` that users of macOS < 13 need to install `clang` via homebrew?
#https://github.com/bitcoin/bitcoin/issues/29918 is an unrelated issue existing on current master, so I think a separate pull request would be more appropriate. Also, I don't have or use macOS/Apple, so someone else creating that pull request would be beneficial.
💬 cbergqvist commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1574574102)
nit: Noticed you renamed `test_node` -> `peer` above (based off https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1531465214) but not inside `test_outbound_eviction_blocks_relay_only()`.
(Was diffing against older head (59affd0bf9042d746a6b32739edb0cd45d28d569)).
Might be worth changing once you rebase on top of #29736.
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1574574102)
nit: Noticed you renamed `test_node` -> `peer` above (based off https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1531465214) but not inside `test_outbound_eviction_blocks_relay_only()`.
(Was diffing against older head (59affd0bf9042d746a6b32739edb0cd45d28d569)).
Might be worth changing once you rebase on top of #29736.
💬 glozow commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2069138726)
concept ACK fwiw
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2069138726)
concept ACK fwiw
💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-2069158282)
OK Luke you've persuaded me. I've re-worked this now to accept `-rpccookieperms=[user|group|others]`.
> nit: Recommend augmenting content in doc/init.md to inform of default cookie file permissions.
Example in this commit https://github.com/tdb3/bitcoin/commit/fdd8a4e70632c74c630caaff5cc36da0c309168e (welcome to cherry pick as desired).
bitcoind --help already states default permissions of 400, so the addition to init.md is author's discretion.
Thansk @tdb3, I've taken your suggestion in
...
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-2069158282)
OK Luke you've persuaded me. I've re-worked this now to accept `-rpccookieperms=[user|group|others]`.
> nit: Recommend augmenting content in doc/init.md to inform of default cookie file permissions.
Example in this commit https://github.com/tdb3/bitcoin/commit/fdd8a4e70632c74c630caaff5cc36da0c309168e (welcome to cherry pick as desired).
bitcoind --help already states default permissions of 400, so the addition to init.md is author's discretion.
Thansk @tdb3, I've taken your suggestion in
...
🚀 glozow merged a pull request: "fuzz: explicitly cap the vsize of RBFs for diagram checks"
(https://github.com/bitcoin/bitcoin/pull/29879)
(https://github.com/bitcoin/bitcoin/pull/29879)
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574610556)
I copied this from the way we do `unique_parents` in orphan parent requests (see the discussion on #19596 about the dynamic memory usage and speed). However I don't mind either way and agree a `std::set` would be simpler, so happy to change if people prefer using a set.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574610556)
I copied this from the way we do `unique_parents` in orphan parent requests (see the discussion on #19596 about the dynamic memory usage and speed). However I don't mind either way and agree a `std::set` would be simpler, so happy to change if people prefer using a set.
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574612750)
Ah, forgot to remove the comment. We need to make time parameterizable across `TxOrphanage` members to test order, so not testing / saving for followup.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574612750)
Ah, forgot to remove the comment. We need to make time parameterizable across `TxOrphanage` members to test order, so not testing / saving for followup.
✅ maflcko closed a pull request: "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner"
(https://github.com/bitcoin/bitcoin/pull/29744)
(https://github.com/bitcoin/bitcoin/pull/29744)
💬 maflcko commented on pull request "lint: Refactor lint checks to reuse exclusion logic; Support running individual lint checks in lint runner":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2069237590)
Closing for now per previous discussion. Feel free to create a new pull request(s) with the behavior changes and an accurate description in your own words. Please try to avoid the use of the word "refactor" for bugfixes or behavior changes.
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2069237590)
Closing for now per previous discussion. Feel free to create a new pull request(s) with the behavior changes and an accurate description in your own words. Please try to avoid the use of the word "refactor" for bugfixes or behavior changes.
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574632413)
I think it's pretty common to name a temporary data structure based on its intended result. But ok, I've changed it to `iters` now.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574632413)
I think it's pretty common to name a temporary data structure based on its intended result. But ok, I've changed it to `iters` now.
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574634137)
Ah that was supposed to be another log, should be fixed now
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1574634137)
Ah that was supposed to be another log, should be fixed now