💬 josibake commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#issuecomment-1532775504)
Concept ACK
I think when I initially looked at this PR the `BitcoinWalletTestFramework` class was breaking `TestShell`, but looking through the PR now, it seems the approach has changed to not use a `BitcoinWalletTestFramework` class. Can you update the description detailing the approach you are now taking?
Did a brief code review and the changes good look good, will test and review once you update the description so I can determine if the code is actually doing what you want it to do :smi
...
(https://github.com/bitcoin/bitcoin/pull/20892#issuecomment-1532775504)
Concept ACK
I think when I initially looked at this PR the `BitcoinWalletTestFramework` class was breaking `TestShell`, but looking through the PR now, it seems the approach has changed to not use a `BitcoinWalletTestFramework` class. Can you update the description detailing the approach you are now taking?
Did a brief code review and the changes good look good, will test and review once you update the description so I can determine if the code is actually doing what you want it to do :smi
...
💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183502224)
I'm trying to reproduce the warning. What version of which compiler do you use? And what flags have you enabled?
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1183502224)
I'm trying to reproduce the warning. What version of which compiler do you use? And what flags have you enabled?
📝 hebasto opened a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561)
For out-of-source builds, the `test/functional/test_runner.py` is supposed to be run from the build directory which allows it to pick the `test/config.ini` file generated by the build system. Currently, it works accidently for the following reasons:
- on POSIX systems, when running a created by Autoconf symlink to the `test/functional/test_runner.py` in the source directory, it actually has the source directory location in the `sys.path`.
- on Windows (the `build_msvc` directory) VS project pu
...
(https://github.com/bitcoin/bitcoin/pull/27561)
For out-of-source builds, the `test/functional/test_runner.py` is supposed to be run from the build directory which allows it to pick the `test/config.ini` file generated by the build system. Currently, it works accidently for the following reasons:
- on POSIX systems, when running a created by Autoconf symlink to the `test/functional/test_runner.py` in the source directory, it actually has the source directory location in the `sys.path`.
- on Windows (the `build_msvc` directory) VS project pu
...
💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1532787387)
Friendly ping @stickies-v :)
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1532787387)
Friendly ping @stickies-v :)
💬 fanquake commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532797807)
cc also @jnewbery @mzumsande @sdaftuar
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532797807)
cc also @jnewbery @mzumsande @sdaftuar
💬 cefikhan commented on pull request "ScriptToUniv can get address of PUBKEY":
(https://github.com/bitcoin/bitcoin/pull/27546#issuecomment-1532800186)
> Concept NACK
>
> Sorry, no. P2PK outputs do not _have_ an address. It's an outdated and confusing practice to refer to them as their corresponding P2PKH address.
as far as my understanding
see we have
private key --> secp256k1 -- > public key --> sha256 --> RIPEMD160 --> Hashed Publickey --> hashed Publickey + checksum = wallet address
and we have the flow of ExtractDestination
PUBKEY -->passed to --> EXTRACT DESTINATION -->PKHash returned
PUBKEYHASH -->passed to --> EXTR
...
(https://github.com/bitcoin/bitcoin/pull/27546#issuecomment-1532800186)
> Concept NACK
>
> Sorry, no. P2PK outputs do not _have_ an address. It's an outdated and confusing practice to refer to them as their corresponding P2PKH address.
as far as my understanding
see we have
private key --> secp256k1 -- > public key --> sha256 --> RIPEMD160 --> Hashed Publickey --> hashed Publickey + checksum = wallet address
and we have the flow of ExtractDestination
PUBKEY -->passed to --> EXTRACT DESTINATION -->PKHash returned
PUBKEYHASH -->passed to --> EXTR
...
💬 dergoegge commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183531240)
Making actual requests from the unit tests is not great.
If you want to add a unit test, you could introduce a way to mock `getaddrinfo` or `AsyncGetAddrInfo` so that you can better test our code in all the relevant scenarios (i.e. getaddrinfo errors/blocking etc.).
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183531240)
Making actual requests from the unit tests is not great.
If you want to add a unit test, you could introduce a way to mock `getaddrinfo` or `AsyncGetAddrInfo` so that you can better test our code in all the relevant scenarios (i.e. getaddrinfo errors/blocking etc.).
💬 dergoegge commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183533168)
nit: Maybe use https://en.cppreference.com/w/cpp/thread/future/wait_until instead?
(https://github.com/bitcoin/bitcoin/pull/27557#discussion_r1183533168)
nit: Maybe use https://en.cppreference.com/w/cpp/thread/future/wait_until instead?
💬 josibake commented on pull request "rpc, p2p: allow `disconnectnode` with subnet":
(https://github.com/bitcoin/bitcoin/pull/26576#issuecomment-1532831791)
Concept ACK
I would recommend updating the description with a more clear use case, as I also wasn't convinced this was a good change until I read through the discussion and saw your comments @brunoerg
> This also seems out of scope for the disconnectnode rpc as it is meant (judging by the name and api right now) to disconnect a single node.
Counter argument: as mentioned in the description, `DisconnectNode` supports subnet as an argument:
https://github.com/bitcoin/bitcoin/blob/38d0
...
(https://github.com/bitcoin/bitcoin/pull/26576#issuecomment-1532831791)
Concept ACK
I would recommend updating the description with a more clear use case, as I also wasn't convinced this was a good change until I read through the discussion and saw your comments @brunoerg
> This also seems out of scope for the disconnectnode rpc as it is meant (judging by the name and api right now) to disconnect a single node.
Counter argument: as mentioned in the description, `DisconnectNode` supports subnet as an argument:
https://github.com/bitcoin/bitcoin/blob/38d0
...
💬 jarolrod commented on pull request "qt: 25.0rc2 translations update":
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1532835443)
is this now pushed to rc3? cc @fanquake @achow101
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1532835443)
is this now pushed to rc3? cc @fanquake @achow101
💬 fanquake commented on pull request "qt: 25.0rc2 translations update":
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1532839992)
> is this now pushed to rc3?
An rc2 hasn't been tagged yet; not sure why it would be pushed to rc3.
(https://github.com/bitcoin/bitcoin/pull/27517#issuecomment-1532839992)
> is this now pushed to rc3?
An rc2 hasn't been tagged yet; not sure why it would be pushed to rc3.
💬 fanquake commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532841412)
Using master (49d543dcaf6ac1b71f8d607dab464a39aff837ac) & GCC 11.3.0 & LTO:
```bash
./univalue/include/univalue.h: In member function 'getInt':
./univalue/include/univalue.h:146:12: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized]
146 | return result;
| ^
./univalue/include/univalue.h:141:9: note: 'result' was declared here
141 | Int result;
| ^
./univalue/include/univalue.h: In member function 'getInt':
./u
...
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532841412)
Using master (49d543dcaf6ac1b71f8d607dab464a39aff837ac) & GCC 11.3.0 & LTO:
```bash
./univalue/include/univalue.h: In member function 'getInt':
./univalue/include/univalue.h:146:12: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized]
146 | return result;
| ^
./univalue/include/univalue.h:141:9: note: 'result' was declared here
141 | Int result;
| ^
./univalue/include/univalue.h: In member function 'getInt':
./u
...
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1170218479)
nit: Seems odd to add a function that is only used in one place. If you decide to keep it, it may be better
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1170218479)
nit: Seems odd to add a function that is only used in one place. If you decide to keep it, it may be better
🤔 MarcoFalke reviewed a pull request: "refactor, kernel: Decouple ArgsManager from blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1390404266)
f59f0c91acb7a35b767bb0dc75ed8b10add81d9f 🙍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: f59f0c91acb7a35b767bb0dc75ed8b10ad
...
(https://github.com/bitcoin/bitcoin/pull/27125#pullrequestreview-1390404266)
f59f0c91acb7a35b767bb0dc75ed8b10add81d9f 🙍
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: f59f0c91acb7a35b767bb0dc75ed8b10ad
...
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183527429)
Unrelated in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: Is there a reason you are sometimes using `gArgs` and then `m_args`? It looks like `m_args` ignores `extra_args` and `gArgs`/`m_node.args` doesn't?
So my preference would be to figure out whether to use `m_node.args` or `m_args`, and then use it consistently?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183527429)
Unrelated in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: Is there a reason you are sometimes using `gArgs` and then `m_args`? It looks like `m_args` ignores `extra_args` and `gArgs`/`m_node.args` doesn't?
So my preference would be to figure out whether to use `m_node.args` or `m_args`, and then use it consistently?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059)
unrelated in 512592cf41ae8fb307bd6eb651e3319e5053851e: Might be good to store a reference to the chain params in m_opts in a previous commit and use it inside the function. Then, while touching all lines here, the now unused params argument could be dropped.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059)
unrelated in 512592cf41ae8fb307bd6eb651e3319e5053851e: Might be good to store a reference to the chain params in m_opts in a previous commit and use it inside the function. Then, while touching all lines here, the now unused params argument could be dropped.
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183561359)
nit (same commit): You can bind blockman to a lambda named `CheckFilterLookups` that binds the arg to avoid touching those lines
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183561359)
nit (same commit): You can bind blockman to a lambda named `CheckFilterLookups` that binds the arg to avoid touching those lines
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183537074)
87999089e1b7794cd595ca19893a383149546087: Can you explain why it is necessary and safe to move this out of the for loop?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183537074)
87999089e1b7794cd595ca19893a383149546087: Can you explain why it is necessary and safe to move this out of the for loop?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183519088)
unrelated nit in a48fd5f3c17a0a6030d0ba47fa42be41f7e4aad9: I wonder what `memory` is being used for, but iwyu seems happy, so this is fine.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183519088)
unrelated nit in a48fd5f3c17a0a6030d0ba47fa42be41f7e4aad9: I wonder what `memory` is being used for, but iwyu seems happy, so this is fine.
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183542168)
nit in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: What is the point of adding 5 LOC that will be removed in the next commit anyway? If you want to keep that, it would be good to also remove the now unused include as well, which you forgot?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183542168)
nit in 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91: What is the point of adding 5 LOC that will be removed in the next commit anyway? If you want to keep that, it would be good to also remove the now unused include as well, which you forgot?