💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1888644663)
I switched to using `std::optional` as the return type. It happily compiles on my end, we'll see what CI thinks...
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1888644663)
I switched to using `std::optional` as the return type. It happily compiles on my end, we'll see what CI thinks...
💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1888672189)
Had to `#include <vector>` to please clang13 (should have included it anyway since it's used).
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1888672189)
Had to `#include <vector>` to please clang13 (should have included it anyway since it's used).
💬 t-bast commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1888673524)
> max_accepted_htlcs is a limit on the number of inbound HTLCs. max_offered_htlc would be some new BOLT-channel policy parameter to limit the number of outbound HTLCs, the idea has floated few times among LN folks to bind max commitment transaction size.
This new `max_offered_htlc` parameter is unnecessary. An HTLC sender unilaterally decides whether they send HTLCs or not, and ensure that they never send more outgoing HTLCs than their `max_accepted_htlcs`. That has always been eclair's behav
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1888673524)
> max_accepted_htlcs is a limit on the number of inbound HTLCs. max_offered_htlc would be some new BOLT-channel policy parameter to limit the number of outbound HTLCs, the idea has floated few times among LN folks to bind max commitment transaction size.
This new `max_offered_htlc` parameter is unnecessary. An HTLC sender unilaterally decides whether they send HTLCs or not, and ensure that they never send more outgoing HTLCs than their `max_accepted_htlcs`. That has always been eclair's behav
...
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450161338)
Oops! Removed.
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450161338)
Oops! Removed.
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450162485)
Added this comment in the legacy test as I agree it'll be convenient to know what coverage we are/aren't losing when deleting a legacy test.
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450162485)
Added this comment in the legacy test as I agree it'll be convenient to know what coverage we are/aren't losing when deleting a legacy test.
🚀 fanquake merged a pull request: "ci: Rename tasks (previous releases, macOS cross)"
(https://github.com/bitcoin/bitcoin/pull/29218)
(https://github.com/bitcoin/bitcoin/pull/29218)
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450164269)
Ah yeah, it's a wallet-specific RPC. I've added a check that the "confirmations" value is 1 on wallet 0 a few lines up, prior to the reorg. Hopefully that also helps make it clear why we're testing this way.
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450164269)
Ah yeah, it's a wallet-specific RPC. I've added a check that the "confirmations" value is 1 on wallet 0 a few lines up, prior to the reorg. Hopefully that also helps make it clear why we're testing this way.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1450168792)
`ShouldFanoutTo` looks fitting for benchmarking, and `partial_sort` had no effect.
I took the former suggestion of course.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1450168792)
`ShouldFanoutTo` looks fitting for benchmarking, and `partial_sort` had no effect.
I took the former suggestion of course.
🚀 fanquake merged a pull request: "build: Bump clang minimum supported version to 14"
(https://github.com/bitcoin/bitcoin/pull/29208)
(https://github.com/bitcoin/bitcoin/pull/29208)
👍 stickies-v approved a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1817832198)
re-ACK c39e82926a3740642f8f1bb72c6b10b67a6dc724
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1817832198)
re-ACK c39e82926a3740642f8f1bb72c6b10b67a6dc724
💬 fanquake commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1888805134)
Guix build (aarch64 & x86_64):
```bash
73062e70b4ec1fa814c0c25f12cbbcfbabfd3a5e82827a3bfeb2f29b30d0b84f guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/SHA256SUMS.part
0965ed91f84129a64b6e0873b5feb182d83cae0dac6d89a51667d5fbe46fc68c guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/bitcoin-a28c6bbde0bc-aarch64-linux-gnu-debug.tar.gz
87ee5bc2103fc10b8e7acbfa776430918538f05fccf024b624f224f73987f745 guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/bitcoin-a28c6bbde0bc-aarch64-linux-gnu.tar.g
...
(https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1888805134)
Guix build (aarch64 & x86_64):
```bash
73062e70b4ec1fa814c0c25f12cbbcfbabfd3a5e82827a3bfeb2f29b30d0b84f guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/SHA256SUMS.part
0965ed91f84129a64b6e0873b5feb182d83cae0dac6d89a51667d5fbe46fc68c guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/bitcoin-a28c6bbde0bc-aarch64-linux-gnu-debug.tar.gz
87ee5bc2103fc10b8e7acbfa776430918538f05fccf024b624f224f73987f745 guix-build-a28c6bbde0bc/output/aarch64-linux-gnu/bitcoin-a28c6bbde0bc-aarch64-linux-gnu.tar.g
...
🤔 dergoegge reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1817887891)
I don't see what this would be useful for, could you elaborate on the motivation?
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1817887891)
I don't see what this would be useful for, could you elaborate on the motivation?
💬 glozow commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1888843388)
https://github.com/bitcoin/bitcoin/actions/runs/7500571397/job/20419528825?pr=29179
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1888843388)
https://github.com/bitcoin/bitcoin/actions/runs/7500571397/job/20419528825?pr=29179
💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1888844067)
CI failure is unrelated, #29234
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1888844067)
CI failure is unrelated, #29234
⚠️ fanquake opened an issue: "rpc: actually deprecate `rpcuser` & `rpcpass`"
(https://github.com/bitcoin/bitcoin/issues/29240)
The logging to output that `Config options rpcuser and rpcpassword will soon be deprecated.` was added ~8 years ago in #7044. However these options still continue to get new usage today, i.e https://github.com/lightningnetwork/lnd/pull/8354#discussion_r1450166947, and it's easy to forget they are considered deprecated, given they aren't under the normal deprecation mechanism/there aren't more verbose warnings.
If we are going to deprecate these options, now seems like a good time to do so. We
...
(https://github.com/bitcoin/bitcoin/issues/29240)
The logging to output that `Config options rpcuser and rpcpassword will soon be deprecated.` was added ~8 years ago in #7044. However these options still continue to get new usage today, i.e https://github.com/lightningnetwork/lnd/pull/8354#discussion_r1450166947, and it's easy to forget they are considered deprecated, given they aren't under the normal deprecation mechanism/there aren't more verbose warnings.
If we are going to deprecate these options, now seems like a good time to do so. We
...
🤔 glozow reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1817851701)
+1 Could you update the OP with motivation for this PR? I didn't see much from the linked PRs
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1817851701)
+1 Could you update the OP with motivation for this PR? I didn't see much from the linked PRs
💬 glozow commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1450185018)
concept ack to having a TestFramework-wide setting to whitelist everybody for immediate tx relay. e5593fc9bf48b27227786e7f4145b58c849367a3 could be its own PR, as I don't think there are any instances where we are specifically making outbound connections and need immediate tx relay.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1450185018)
concept ack to having a TestFramework-wide setting to whitelist everybody for immediate tx relay. e5593fc9bf48b27227786e7f4145b58c849367a3 could be its own PR, as I don't think there are any instances where we are specifically making outbound connections and need immediate tx relay.
💬 glozow commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1450191936)
43614707c4fe9ce91bb90b959574ee4dbf6f7b05
Why is the approach to add "in" and "out" instead of just having specified permissions apply to both? Are there instances where you want to specify different permissions for the same address depending on the connection direction?
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1450191936)
43614707c4fe9ce91bb90b959574ee4dbf6f7b05
Why is the approach to add "in" and "out" instead of just having specified permissions apply to both? Are there instances where you want to specify different permissions for the same address depending on the connection direction?
✅ fanquake closed an issue: "`./configure` fails for clang-14 on Ubuntu 23.10"
(https://github.com/bitcoin/bitcoin/issues/29161)
(https://github.com/bitcoin/bitcoin/issues/29161)
💬 fanquake commented on issue "`./configure` fails for clang-14 on Ubuntu 23.10":
(https://github.com/bitcoin/bitcoin/issues/29161#issuecomment-1888920640)
Going to close this as wontfix. If someone is using an operating system that is modern enough to be shipping GCC 13.2 and Clang 16 as the deafult compilers, then I'd suggest using either of those as their compiler. Also given that this is a bug in Clang, but only when coupled with a certain version of libstdc++, it's not easy for us to "fix". An alternative is to try with libc++. For older systems, clang-14 remains supported (for now).
(https://github.com/bitcoin/bitcoin/issues/29161#issuecomment-1888920640)
Going to close this as wontfix. If someone is using an operating system that is modern enough to be shipping GCC 13.2 and Clang 16 as the deafult compilers, then I'd suggest using either of those as their compiler. Also given that this is a bug in Clang, but only when coupled with a certain version of libstdc++, it's not easy for us to "fix". An alternative is to try with libc++. For older systems, clang-14 remains supported (for now).