💬 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?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183565479)
f665024be7e5cc0a91ac7ff5779209313453a043: Wrong commit? How is this not missing from 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183565479)
f665024be7e5cc0a91ac7ff5779209313453a043: Wrong commit? How is this not missing from 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91?
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183550132)
3b9d6b06407f79d87b3318ecaaaca696b0dd1e68: Why?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183550132)
3b9d6b06407f79d87b3318ecaaaca696b0dd1e68: Why?
💬 MarcoFalke commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532881584)
My previous reply should be valid for that one as well
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532881584)
My previous reply should be valid for that one as well
💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183583641)
It wasn't possible to do that in https://github.com/bitcoin/bitcoin/commit/7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 because `BlockFileSeq` wasn't part of `BlockManager` then. One could rearrange the commits though and have 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 after the commit that moves stuff into BlockManager.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183583641)
It wasn't possible to do that in https://github.com/bitcoin/bitcoin/commit/7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 because `BlockFileSeq` wasn't part of `BlockManager` then. One could rearrange the commits though and have 7e0e84c1a3587f8b2ffac00f647d1e2d17febb91 after the commit that moves stuff into BlockManager.