💬 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.
💬 fanquake commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532892309)
Yea. I don't really have an intention of fixing these ones, just keeping tabs so that if they do get reported, we can point to a thread.
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1532892309)
Yea. I don't really have an intention of fixing these ones, just keeping tabs so that if they do get reported, we can point to a thread.
🤔 glozow reviewed a pull request: "doc: clarify processing of mempool-msgs when NODE_BLOOM"
(https://github.com/bitcoin/bitcoin/pull/27559#pullrequestreview-1410732256)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
(https://github.com/bitcoin/bitcoin/pull/27559#pullrequestreview-1410732256)
ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
🚀 glozow merged a pull request: "doc: clarify processing of mempool-msgs when NODE_BLOOM"
(https://github.com/bitcoin/bitcoin/pull/27559)
(https://github.com/bitcoin/bitcoin/pull/27559)
💬 mzumsande commented on pull request "doc: clarify processing of mempool-msgs when NODE_BLOOM":
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532929566)
Post-merge ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
(https://github.com/bitcoin/bitcoin/pull/27559#issuecomment-1532929566)
Post-merge ACK 4581a682d2d1fdd0e56fb4a56e6228be878a04a3
💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183625464)
if found, `std::search` returns the Iterator to the beginning of the first occurrence of the sequence, so this should be `!= key.begin` or `== key.end()`.
Crafted a quick test to check it https://github.com/furszy/bitcoin-core/commit/f8a7c0670f95b317c46b01855c4c0f0db7ba7714 (no need to add it, I was just double checking it)
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183625464)
if found, `std::search` returns the Iterator to the beginning of the first occurrence of the sequence, so this should be `!= key.begin` or `== key.end()`.
Crafted a quick test to check it https://github.com/furszy/bitcoin-core/commit/f8a7c0670f95b317c46b01855c4c0f0db7ba7714 (no need to add it, I was just double checking it)