💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2156780094)
nit: requires `#include <utility>`
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2156780094)
nit: requires `#include <utility>`
💬 fanquake commented on pull request "doc: Remove build instruction for running `clang-tidy`":
(https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2988172883)
> Also, would be good to mention that this will now print errors about the missing auto-generated files.
Yea, I think we could actually revert this. It mostly *works*, but as you say, now produces errors due to missing files, i.e:
```bash
/root/ci_scratch/src/test/sighash_tests.cpp:13:10: error: 'test/data/sighash.json.h' file not found [clang-diagnostic-error]
13 | #include <test/data/sighash.json.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
694 warnings and 1 error generated.
```
...
(https://github.com/bitcoin/bitcoin/pull/32662#issuecomment-2988172883)
> Also, would be good to mention that this will now print errors about the missing auto-generated files.
Yea, I think we could actually revert this. It mostly *works*, but as you say, now produces errors due to missing files, i.e:
```bash
/root/ci_scratch/src/test/sighash_tests.cpp:13:10: error: 'test/data/sighash.json.h' file not found [clang-diagnostic-error]
13 | #include <test/data/sighash.json.h>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
694 warnings and 1 error generated.
```
...
💬 bigshiny90 commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021687570)
@SomberNight
Compilation of potential fixes for PR #32844:
## 1. Witness nonce issue
The witness commitment wasn't being calculated correctly. Added in `src/rpc/txoutproof.cpp`:
First, add required includes at the top:
```cpp
#include <hash.h>
#include <logging.h>
```
Then after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_ge
...
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3021687570)
@SomberNight
Compilation of potential fixes for PR #32844:
## 1. Witness nonce issue
The witness commitment wasn't being calculated correctly. Added in `src/rpc/txoutproof.cpp`:
First, add required includes at the top:
```cpp
#include <hash.h>
#include <logging.h>
```
Then after line 220:
```cpp
const auto wtxid_root = merkleBlock.m_wtxid_tree.ExtractMatches(wtxid_matches, wtxid_indexes);
// Extract witness nonce from coinbase input
if (merkleBlock.m_gentx->vin.empty() || merkleBlock.m_ge
...
💬 fanquake commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027713936)
Yea, I'm not sure what is the best approach here. However I don't think a good way forward is PR's like this, based on IWYU output, which simultaneously ignore other IWYU output (and no explanation for devs that might otherwise try to make changes based on the output). I think any changes should be a part of #31308.
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027713936)
Yea, I'm not sure what is the best approach here. However I don't think a good way forward is PR's like this, based on IWYU output, which simultaneously ignore other IWYU output (and no explanation for devs that might otherwise try to make changes based on the output). I think any changes should be a part of #31308.
💬 maflcko commented on pull request "refactor: Drop unused `#include <boost/operators.hpp>`":
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027737893)
>I think any changes should be a part of #31308.
I'd say it is also fine to do it in smaller steps, but no strong opinion.
(https://github.com/bitcoin/bitcoin/pull/32668#issuecomment-3027737893)
>I think any changes should be a part of #31308.
I'd say it is also fine to do it in smaller steps, but no strong opinion.
✅ hebasto closed a pull request: "refactor: Drop unused `#include <boost/operators.hpp>`"
(https://github.com/bitcoin/bitcoin/pull/32668)
(https://github.com/bitcoin/bitcoin/pull/32668)
💬 stickies-v commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2179755821)
In 7c4eb58fb0:
I don't really see the point of using `util::Overloaded` here when we're always doing a `Wtxid` query?
This looks more straightforward to me:
<details>
<summary>git diff on 7c4eb58fb0</summary>
```diff
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index a16490ceb9..87a47b30ee 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -9,7 +9,6 @@
#include <consensus/validation.h>
#include <logging.h>
...
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2179755821)
In 7c4eb58fb0:
I don't really see the point of using `util::Overloaded` here when we're always doing a `Wtxid` query?
This looks more straightforward to me:
<details>
<summary>git diff on 7c4eb58fb0</summary>
```diff
diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp
index a16490ceb9..87a47b30ee 100644
--- a/src/node/txdownloadman_impl.cpp
+++ b/src/node/txdownloadman_impl.cpp
@@ -9,7 +9,6 @@
#include <consensus/validation.h>
#include <logging.h>
...
💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216982231)
nit: could do IWYU I guess, even though this wasn't followed that well in the index cpp files.
```
#include <dbwrapper.h>
#include <interfaces/types.h>
```
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216982231)
nit: could do IWYU I guess, even though this wasn't followed that well in the index cpp files.
```
#include <dbwrapper.h>
#include <interfaces/types.h>
```
⚠️ totdking opened an issue: "A missing import to the src/chainparamsbase.h"
(https://github.com/bitcoin/bitcoin/issues/33019)
Tried running the make -j $CORES as the bitcoin repo is important to what I'm working with.
The import that is missing is the ```#include <cstdint>```, it gave a plethora of errors.
This is not a bug but i just wanted the community to have a peaceful and seamless integration while building the repo
(https://github.com/bitcoin/bitcoin/issues/33019)
Tried running the make -j $CORES as the bitcoin repo is important to what I'm working with.
The import that is missing is the ```#include <cstdint>```, it gave a plethora of errors.
This is not a bug but i just wanted the community to have a peaceful and seamless integration while building the repo
📝 hebasto opened a pull request: "refactor: Move `FreespaceChecker` class into its own module"
(https://github.com/bitcoin-core/gui/pull/881)
The MOC compiler in older versions of Qt 6 fails to parse `qt/intro.cpp`, as noted in [this comment](https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233).
This PR proposes a move-only refactoring to simplify the source structure by eliminating the need for the inline `#include <qt/intro.moc>`, thereby effectively working around the issue.
(https://github.com/bitcoin-core/gui/pull/881)
The MOC compiler in older versions of Qt 6 fails to parse `qt/intro.cpp`, as noted in [this comment](https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233).
This PR proposes a move-only refactoring to simplify the source structure by eliminating the need for the inline `#include <qt/intro.moc>`, thereby effectively working around the issue.