💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1574722548)
We already print settings somewhere else, and this isn't where the actual permissions are being changed.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1574722548)
We already print settings somewhere else, and this isn't where the actual permissions are being changed.
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1574723868)
nit: "all" seems better than "others" here IMO
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1574723868)
nit: "all" seems better than "others" here IMO
💬 hebasto commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2115593433)
@dominicusadinfinitum
> where or how do I get to the debug.log?
In your data directory. Please consult the https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#data-directory-layout.
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2115593433)
@dominicusadinfinitum
> where or how do I get to the debug.log?
In your data directory. Please consult the https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#data-directory-layout.
💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115593653)
> Could this be a case for a clang-tidy plugin?
I considered this, but I don't think so. UniValue copies are reasonable in many cases, but our use of them often lends itself to moving. So we can't detect and disable copies as a rule, and (I assume) if clang-tidy detect and suggest possible moves as optimizations, it would be offering a generic version of that already.
Here's an upstream discussion about it that has apparently gone stale: https://github.com/llvm/llvm-project/issues/53489
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115593653)
> Could this be a case for a clang-tidy plugin?
I considered this, but I don't think so. UniValue copies are reasonable in many cases, but our use of them often lends itself to moving. So we can't detect and disable copies as a rule, and (I assume) if clang-tidy detect and suggest possible moves as optimizations, it would be offering a generic version of that already.
Here's an upstream discussion about it that has apparently gone stale: https://github.com/llvm/llvm-project/issues/53489
💬 hebasto commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115594712)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115594712)
Concept ACK.
🤔 vasild reviewed a pull request: "net: Replace libnatpmp with built-in PCP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2061123015)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2061123015)
Concept ACK
💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603565742)
The following patch uses a netlink socket to get the information from the kernel, that is supported on (at least) Linux and FreeBSD>=13.2:
<details>
<summary>[patch] get default gateway using a netlink socket</summary>
```diff
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -1,25 +1,34 @@
// Copyright (c) 2012-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licen
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603565742)
The following patch uses a netlink socket to get the information from the kernel, that is supported on (at least) Linux and FreeBSD>=13.2:
<details>
<summary>[patch] get default gateway using a netlink socket</summary>
```diff
--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -1,25 +1,34 @@
// Copyright (c) 2012-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licen
...
💬 hebasto commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115606470)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115606470)
Concept ACK.
💬 ajtowns commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2115614319)
Bumped past #29086
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2115614319)
Bumped past #29086
💬 ryanofsky commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115622164)
> Could this be a case for a clang-tidy plugin?
I think the ideal thing to do for types that are potentially expensive to copy is to disable implicit copies, but provide explicit `T Copy() const` and `void CopyFrom(const T&)` methods so copies can be made where needed. (There was a `DISABLE_IMPLICIT_COPIES` macro proposed in #24641 that tried to do this, but it didn't seem possible to generalize the implementation so we never added it.) It think it would be probably be reasonable to allow uni
...
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115622164)
> Could this be a case for a clang-tidy plugin?
I think the ideal thing to do for types that are potentially expensive to copy is to disable implicit copies, but provide explicit `T Copy() const` and `void CopyFrom(const T&)` methods so copies can be made where needed. (There was a `DISABLE_IMPLICIT_COPIES` macro proposed in #24641 that tried to do this, but it didn't seem possible to generalize the implementation so we never added it.) It think it would be probably be reasonable to allow uni
...
💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2115624871)
From a more general wallet perspective if we're assuming ~110vbytes of overhead per additional transaction required to match an input/output set, picking 10kvB as the new limit vs 100kvB results in 10*110=1,100vb, or ~1% additional overhead vs 100kvB. At 5kvB you'd be receiving ~2% penalty, and so on.
FWIW 10kvB also seems like plenty of headroom for practical LN channels with >60-HTLCs-per-direction, but below today's spec limit.
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2115624871)
From a more general wallet perspective if we're assuming ~110vbytes of overhead per additional transaction required to match an input/output set, picking 10kvB as the new limit vs 100kvB results in 10*110=1,100vb, or ~1% additional overhead vs 100kvB. At 5kvB you'd be receiving ~2% penalty, and so on.
FWIW 10kvB also seems like plenty of headroom for practical LN channels with >60-HTLCs-per-direction, but below today's spec limit.
👍 hebasto approved a pull request: "bench: enable wallet creation benchmarks on all platforms"
(https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2061248474)
ACK 7323bf6a2ecc3a2f1e2fdebce53b961767b06e08.
Maybe deduplicate code:
```diff
--- a/src/bench/wallet_create.cpp
+++ b/src/bench/wallet_create.cpp
@@ -35,8 +35,8 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
std::vector<bilingual_str> warnings;
int round = 0;
- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round, random.rand32()).c_str();
bench.run([&] {
+ auto wallet_path = test_setup->m_path_root / s
...
(https://github.com/bitcoin/bitcoin/pull/30122#pullrequestreview-2061248474)
ACK 7323bf6a2ecc3a2f1e2fdebce53b961767b06e08.
Maybe deduplicate code:
```diff
--- a/src/bench/wallet_create.cpp
+++ b/src/bench/wallet_create.cpp
@@ -35,8 +35,8 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
std::vector<bilingual_str> warnings;
int round = 0;
- fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round, random.rand32()).c_str();
bench.run([&] {
+ auto wallet_path = test_setup->m_path_root / s
...
💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1603675011)
Added a comment.
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1603675011)
Added a comment.
💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2115666825)
`c5f9afd946...d35ba1b3f1`: add a comment as suggested above
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2115666825)
`c5f9afd946...d35ba1b3f1`: add a comment as suggested above
🤔 furszy reviewed a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2061247415)
> @furszy Were you able to identify few connections that were dropped in logs of this CI run? https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200
Just one. One of the `P2PInterface` connections I create on the test side gets disconnected after advancing the node time prior to connecting the test nodes again. Need to expand the complete CI logs to find it.
But the fragility is easy to reproduce. Just launch a thread that disconnects a node before calling `connect_nodes()`.
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2061247415)
> @furszy Were you able to identify few connections that were dropped in logs of this CI run? https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200
Just one. One of the `P2PInterface` connections I create on the test side gets disconnected after advancing the node time prior to connecting the test nodes again. Need to expand the complete CI logs to find it.
But the fragility is easy to reproduce. Just launch a thread that disconnects a node before calling `connect_nodes()`.
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603654247)
> Looking at the sample output of `subversion` - `"subversion": "\/Satoshi:25.1.0\/"`, it doesn't seem unique enough because multiple nodes can be running the same version. Won't this cause issues in `find_conn` later?
The test framework appends the node number to the user agent string. See [test_node](https://github.com/bitcoin/bitcoin/blob/2f53f2273da020d7fabd7c65a1bc7e69a31249b2/test/functional/test_framework/test_node.py#L109).
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603654247)
> Looking at the sample output of `subversion` - `"subversion": "\/Satoshi:25.1.0\/"`, it doesn't seem unique enough because multiple nodes can be running the same version. Won't this cause issues in `find_conn` later?
The test framework appends the node number to the user agent string. See [test_node](https://github.com/bitcoin/bitcoin/blob/2f53f2273da020d7fabd7c65a1bc7e69a31249b2/test/functional/test_framework/test_node.py#L109).
💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603657305)
> Might as well make these calls once and store in variable instead of finding 3 times? Unless I'm missing something that requires these calls to be made every time.
The code waits until the data arrives. These requires polling for updates.
We could couple the checks in this way:
```python3
self.wait_until(lambda: (peer := find_conn(from_connection, to_connection_subver, inbound=False)) is not None
and peer['version'] != 0
...
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603657305)
> Might as well make these calls once and store in variable instead of finding 3 times? Unless I'm missing something that requires these calls to be made every time.
The code waits until the data arrives. These requires polling for updates.
We could couple the checks in this way:
```python3
self.wait_until(lambda: (peer := find_conn(from_connection, to_connection_subver, inbound=False)) is not None
and peer['version'] != 0
...
💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115677153)
Applied, thanks @hebasto 👍🏼.
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115677153)
Applied, thanks @hebasto 👍🏼.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603770501)
Huh interesting. i didn't know netlink worked for multiple operating systems, that's much better.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603770501)
Huh interesting. i didn't know netlink worked for multiple operating systems, that's much better.
💬 ryanofsky commented on pull request "JSON-RPC request Content-Type is application/json":
(https://github.com/bitcoin/bitcoin/pull/29946#issuecomment-2115854243)
Code review ACK f90a84d61505443fd3bb83253c091590b3dc7f45, but I think it would be helpful to change description to something like "doc: specify json content type in rpc examples" because the current description doesn't make it obvious that this is a documentation change, not a change in behavior.
PR also needs to be rebased since #27101 was just merged and it conflicts
(https://github.com/bitcoin/bitcoin/pull/29946#issuecomment-2115854243)
Code review ACK f90a84d61505443fd3bb83253c091590b3dc7f45, but I think it would be helpful to change description to something like "doc: specify json content type in rpc examples" because the current description doesn't make it obvious that this is a documentation change, not a change in behavior.
PR also needs to be rebased since #27101 was just merged and it conflicts