💬 willcl-ark commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1147454419)
I think this is totally fine as is here for now, but in the future we may want to put these all in a list/enum
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1147454419)
I think this is totally fine as is here for now, but in the future we may want to put these all in a list/enum
💬 willcl-ark commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1147486821)
Instead of hardcoding the hrp here, why not have `check_bech32_decode()` take `hrp` as a parameter, then we can add `bc1` test vectors instead of being limited to `tb1`?
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1147486821)
Instead of hardcoding the hrp here, why not have `check_bech32_decode()` take `hrp` as a parameter, then we can add `bc1` test vectors instead of being limited to `tb1`?
💬 willcl-ark commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1147460292)
I may be mistaken but I believe that actually this [_should_](https://peps.python.org/pep-0008/#blank-lines) have 1 line gap, as it's a member function? But its OK, I wouldn't change and invalidate the ACKS you've got already! (I configured my editor to show PEP8 diagnostics so I can view them directly as I'm editing).
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1147460292)
I may be mistaken but I believe that actually this [_should_](https://peps.python.org/pep-0008/#blank-lines) have 1 line gap, as it's a member function? But its OK, I wouldn't change and invalidate the ACKS you've got already! (I configured my editor to show PEP8 diagnostics so I can view them directly as I'm editing).
🚀 fanquake merged a pull request: "test: Support decoding segwit address in address_to_scriptpubkey()"
(https://github.com/bitcoin/bitcoin/pull/27269)
(https://github.com/bitcoin/bitcoin/pull/27269)
🚀 fanquake merged a pull request: "ci: Cache more stuff in the ci images: msan, iwyu, pip, sdks"
(https://github.com/bitcoin/bitcoin/pull/27028)
(https://github.com/bitcoin/bitcoin/pull/27028)
⚠️ MarcoFalke opened an issue: "ci: Cleanup ci/test/01_base_install.sh"
(https://github.com/bitcoin/bitcoin/issues/27321)
### Motivation
Currently `ci/test/01_base_install.sh` has the following issues:
* Two names to alias `bash -c`: `CI_EXEC_ROOT`, and `CI_EXEC`
* The aliases are not needed, as explained in the comment in the file
Thus, they should be removed, or at least de-duplicated. The whole file is already run in bash, so the only place where `bash -c "..."` is needed is in the `apt install` step to properly pass the packages string with spaces.
### Possible solution
* Remove or de-duplicate `CI_EX
...
(https://github.com/bitcoin/bitcoin/issues/27321)
### Motivation
Currently `ci/test/01_base_install.sh` has the following issues:
* Two names to alias `bash -c`: `CI_EXEC_ROOT`, and `CI_EXEC`
* The aliases are not needed, as explained in the comment in the file
Thus, they should be removed, or at least de-duplicated. The whole file is already run in bash, so the only place where `bash -c "..."` is needed is in the `apt install` step to properly pass the packages string with spaces.
### Possible solution
* Remove or de-duplicate `CI_EX
...
💬 vasild commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1147543379)
Alas that does not work if the condition includes function calls.
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1147543379)
Alas that does not work if the condition includes function calls.
💬 willcl-ark commented on issue "core stops to run with `Failed to read block` error":
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1482781323)
Do you think it's something you will try again in the future? It would be good to get reproduction steps if possible...
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1482781323)
Do you think it's something you will try again in the future? It would be good to get reproduction steps if possible...
📝 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.
-
...