📝 jonatack opened a pull request: "move-only: IsDeprecatedRPCEnabled to rpc/util to fix CI builds when called from wallet"
(https://github.com/bitcoin/bitcoin/pull/27322)
While deprecating the remaining wallet RPC "warning" fields in #27279, the CI reported `undefined reference to IsDeprecatedRPCEnabled in libbitcoin_wallet` compiler errors when this function was called from wallet RPCs.
Fix this by moving the function from `rpc/server` to `rpc/util`, which by looking at `doc/design/librairies.md` and `src/Makefile.am` looks like the right place for it. Happy to change approach per reviewer feedback.
Cherry-picked from #27279, where the relevant CI is now
...
(https://github.com/bitcoin/bitcoin/pull/27322)
While deprecating the remaining wallet RPC "warning" fields in #27279, the CI reported `undefined reference to IsDeprecatedRPCEnabled in libbitcoin_wallet` compiler errors when this function was called from wallet RPCs.
Fix this by moving the function from `rpc/server` to `rpc/util`, which by looking at `doc/design/librairies.md` and `src/Makefile.am` looks like the right place for it. Happy to change approach per reviewer feedback.
Cherry-picked from #27279, where the relevant CI is now
...
💬 jonatack commented on issue "wallet_create_tx.py "Not solvable pre-selected input" exception":
(https://github.com/bitcoin/bitcoin/issues/27316#issuecomment-1482789635)
Same error reported by CI in https://cirrus-ci.com/task/5776249860128768?logs=functional_tests#L2682.
```
[default_wallet] AddToWallet 87b8816da11c3e05844802cf2ca4ac0fdcd605d0b53ae046f470264aec116878
test 2023-03-23T23:35:57.593000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_fra
...
(https://github.com/bitcoin/bitcoin/issues/27316#issuecomment-1482789635)
Same error reported by CI in https://cirrus-ci.com/task/5776249860128768?logs=functional_tests#L2682.
```
[default_wallet] AddToWallet 87b8816da11c3e05844802cf2ca4ac0fdcd605d0b53ae046f470264aec116878
test 2023-03-23T23:35:57.593000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_fra
...
💬 virtu commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1147575925)
I can imagine some use cases where having consistent netgroup identifiers would be advantageous. Unless `NetGroupManager::GetGroup()` adds too much overhead, they seem preferable.
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1147575925)
I can imagine some use cases where having consistent netgroup identifiers would be advantageous. Unless `NetGroupManager::GetGroup()` adds too much overhead, they seem preferable.
💬 furszy commented on issue "wallet_create_tx.py "Not solvable pre-selected input" exception":
(https://github.com/bitcoin/bitcoin/issues/27316#issuecomment-1482795409)
@jonatack https://github.com/bitcoin/bitcoin/pull/27318
(https://github.com/bitcoin/bitcoin/issues/27316#issuecomment-1482795409)
@jonatack https://github.com/bitcoin/bitcoin/pull/27318
💬 MarcoFalke commented on pull request "move-only: IsDeprecatedRPCEnabled to rpc/util to fix CI builds when called from wallet":
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482807752)
lgtm
Maybe explain that this moves it from `node` to `common`?
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482807752)
lgtm
Maybe explain that this moves it from `node` to `common`?
📝 MarcoFalke opened a pull request: "ci: Add missing libclang-rt-dev package"
(https://github.com/bitcoin/bitcoin/pull/27323)
This is required, due to `--no-install-recommends` in `ci/test/01_base_install.sh`, I presume.
If not added, this will cause issues if the CI image is bumped, for example, to Ubuntu Lunar.
Asan Lunar:
```
configure:22272: clang++ -std=c++20 -o conftest -g -O2 -DARENA_DEBUG -DDEBUG_LOCKORDER -fsanitize=address,integer,undefined conftest.cpp >&5
/usr/bin/ld: cannot find /usr/lib/llvm-15/lib/clang/15.0.7/lib/linux/libclang_rt.asan_static-x86_64.a: No such file or directory
/usr/bin/l
...
(https://github.com/bitcoin/bitcoin/pull/27323)
This is required, due to `--no-install-recommends` in `ci/test/01_base_install.sh`, I presume.
If not added, this will cause issues if the CI image is bumped, for example, to Ubuntu Lunar.
Asan Lunar:
```
configure:22272: clang++ -std=c++20 -o conftest -g -O2 -DARENA_DEBUG -DDEBUG_LOCKORDER -fsanitize=address,integer,undefined conftest.cpp >&5
/usr/bin/ld: cannot find /usr/lib/llvm-15/lib/clang/15.0.7/lib/linux/libclang_rt.asan_static-x86_64.a: No such file or directory
/usr/bin/l
...
💬 virtu commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1482823973)
ACK [6de4fc7](https://github.com/bitcoin/bitcoin/commit/6de4fc73c935baa2cc6f54dd57c8e57a5df3c7ae)
Code reviewed tracepoints, tests, and demo script. Tracepoint placement looks good, so do tests. Successfully ran functional tests and the demo script.
Some points:
- Any reason why only `inbound_connection` and `outbound_connection` explicitly cast all arguments in the demo script?
- I think some of the format specifiers in the demo script are lightly off, leading to outputs with negative n
...
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1482823973)
ACK [6de4fc7](https://github.com/bitcoin/bitcoin/commit/6de4fc73c935baa2cc6f54dd57c8e57a5df3c7ae)
Code reviewed tracepoints, tests, and demo script. Tracepoint placement looks good, so do tests. Successfully ran functional tests and the demo script.
Some points:
- Any reason why only `inbound_connection` and `outbound_connection` explicitly cast all arguments in the demo script?
- I think some of the format specifiers in the demo script are lightly off, leading to outputs with negative n
...
💬 jonatack commented on pull request "move-only: IsDeprecatedRPCEnabled to rpc/util to fix CI builds when called from wallet":
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482827329)
Thanks @MarcoFalke, good idea, updated the PR description and will update the commit message shortly.
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482827329)
Thanks @MarcoFalke, good idea, updated the PR description and will update the commit message shortly.
💬 MarcoFalke commented on pull request "ci: Add missing libclang-rt-dev package":
(https://github.com/bitcoin/bitcoin/pull/27323#issuecomment-1482831967)
Ah sorry. This requires bumping the image at the same time. Closing.
(https://github.com/bitcoin/bitcoin/pull/27323#issuecomment-1482831967)
Ah sorry. This requires bumping the image at the same time. Closing.
✅ MarcoFalke closed a pull request: "ci: Add missing libclang-rt-dev package"
(https://github.com/bitcoin/bitcoin/pull/27323)
(https://github.com/bitcoin/bitcoin/pull/27323)
💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1147619018)
> Alas that does not work if the condition includes function calls.
Why not? From the GDB docs:
> Break conditions can have side effects, and may even call functions in your program.
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1147619018)
> Alas that does not work if the condition includes function calls.
Why not? From the GDB docs:
> Break conditions can have side effects, and may even call functions in your program.
💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147633787)
Just a suggestion to make the code a lit bit cleaner, perhaps we don't need to create `position` and `nId` again after the loop.
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index cdfd079fc..30ce2cadc 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -757,10 +757,10 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
// Iterate over the positions of that bucket, starting at the initial one,
// and looping around.
-
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147633787)
Just a suggestion to make the code a lit bit cleaner, perhaps we don't need to create `position` and `nId` again after the loop.
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index cdfd079fc..30ce2cadc 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -757,10 +757,10 @@ std::pair<CAddress, NodeSeconds> AddrManImpl::Select_(bool new_only, std::option
// Iterate over the positions of that bucket, starting at the initial one,
// and looping around.
-
...
💬 MarcoFalke commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482879003)
> Ran into a failure (with the above branch, and NO_BDB=1), on aarch64:
Does this happen on master as well?
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482879003)
> Ran into a failure (with the above branch, and NO_BDB=1), on aarch64:
Does this happen on master as well?
💬 MarcoFalke commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482882959)
See also https://github.com/bitcoin/bitcoin/pull/26188
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1482882959)
See also https://github.com/bitcoin/bitcoin/pull/26188
💬 brunoerg commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147671339)
About the optimization in https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132629513, couldn't we simply use a vector to store the visited buckets?
just an example:
```cpp
std::vector<int> buckets_visited;
while (1) {
// Pick a bucket, and an initial position in that bucket.
int bucket = insecure_rand.randrange(bucket_count);
if (std::find(buckets_visited.begin(), buckets_visited.end(), bucket) != buckets_visited.end()) {
continue;
} els
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1147671339)
About the optimization in https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132629513, couldn't we simply use a vector to store the visited buckets?
just an example:
```cpp
std::vector<int> buckets_visited;
while (1) {
// Pick a bucket, and an initial position in that bucket.
int bucket = insecure_rand.randrange(bucket_count);
if (std::find(buckets_visited.begin(), buckets_visited.end(), bucket) != buckets_visited.end()) {
continue;
} els
...
💬 jonatack commented on pull request "move-only: IsDeprecatedRPCEnabled to rpc/util to fix CI builds when called from wallet":
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482910548)
PR description and commit message updated per https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482807752 (thanks @MarcoFalke).
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482910548)
PR description and commit message updated per https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1482807752 (thanks @MarcoFalke).
💬 sipa commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147676124)
I don't think there is a need for these `is_rfc8439` values, because overflowing the block counter in RFC8439 mode is simply not allowed. If it happens, both incrementing j13 or not incrementing it are wrong. If you want to do anything, it should be an assertion failure.
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147676124)
I don't think there is a need for these `is_rfc8439` values, because overflowing the block counter in RFC8439 mode is simply not allowed. If it happens, both incrementing j13 or not incrementing it are wrong. If you want to do anything, it should be an assertion failure.
💬 sipa commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147677294)
I think this variable can be constant (and if it isn't, it probably shouldn't be a global). Also, globals should be upper case.
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147677294)
I think this variable can be constant (and if it isn't, it probably shouldn't be a global). Also, globals should be upper case.
💬 sipa commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147678589)
`static const` here as well?
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1147678589)
`static const` here as well?
💬 ryanofsky commented on pull request "system: allow GUI to initialize default data dir":
(https://github.com/bitcoin/bitcoin/pull/27273#discussion_r1147632734)
In commit "system: allow GUI to initialize default data dir" (7a9b786e00a5275ba012bc37a3796d1d35491161)
This `if` statement no longer does anything and could be dropped. Just calling `InitDefaultDataDir` unconditionally would have the same effect and be easier to understand. Also the comment above needs to be updated. Could replace "Only override -datadir if different from the default, to make it possible [...]" with "Set chosen directory as the default datadir. It is possible [...]"
(https://github.com/bitcoin/bitcoin/pull/27273#discussion_r1147632734)
In commit "system: allow GUI to initialize default data dir" (7a9b786e00a5275ba012bc37a3796d1d35491161)
This `if` statement no longer does anything and could be dropped. Just calling `InitDefaultDataDir` unconditionally would have the same effect and be easier to understand. Also the comment above needs to be updated. Could replace "Only override -datadir if different from the default, to make it possible [...]" with "Set chosen directory as the default datadir. It is possible [...]"