💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1660599083)
Rebased and switched to `Result<void>`.
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1660599083)
Rebased and switched to `Result<void>`.
💬 MarcoFalke commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660604025)
re-ACK e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518 🗞
<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: re-ACK e8a3a0a0d761c8544e2
...
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1660604025)
re-ACK e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518 🗞
<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: re-ACK e8a3a0a0d761c8544e2
...
💬 fanquake commented on pull request "doc: Clarify that -fstack-reuse=all bugs exist on all versions of GCC":
(https://github.com/bitcoin/bitcoin/pull/28105#issuecomment-1660609740)
I see the commentary on the GCC bug, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348#c30, that someone claims the issue still exists, but the test-case no-longer works. Should we add a note to our test case to note that it's no-longer useful post GCC 12?
(https://github.com/bitcoin/bitcoin/pull/28105#issuecomment-1660609740)
I see the commentary on the GCC bug, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348#c30, that someone claims the issue still exists, but the test-case no-longer works. Should we add a note to our test case to note that it's no-longer useful post GCC 12?
🚀 fanquake merged a pull request: "test, rpc: invalid sighashtype coverage"
(https://github.com/bitcoin/bitcoin/pull/28166)
(https://github.com/bitcoin/bitcoin/pull/28166)
💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660615210)
ACK fa8d99d8932df08320f1bfb4e8f1d4b4169466fd (first commit)
I have no issues with Rust, or using it in the CI; we already do in other CIs in the Bitcoin Core project. Saying that, I'm not sure if we want to introduce a 4th way to do linting in this repo (yet). One potential alternative, if we go further down the clang-tidy route, is the `<filesystem>` linter here: https://github.com/fanquake/bitcoin/commits/integrate_thecharlatan_filesystem, put together by @TheCharlatan.
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660615210)
ACK fa8d99d8932df08320f1bfb4e8f1d4b4169466fd (first commit)
I have no issues with Rust, or using it in the CI; we already do in other CIs in the Bitcoin Core project. Saying that, I'm not sure if we want to introduce a 4th way to do linting in this repo (yet). One potential alternative, if we go further down the clang-tidy route, is the `<filesystem>` linter here: https://github.com/fanquake/bitcoin/commits/integrate_thecharlatan_filesystem, put together by @TheCharlatan.
✅ fanquake closed an issue: "p2p_getaddr_caching.py failure in TSan CI"
(https://github.com/bitcoin/bitcoin/issues/28133)
(https://github.com/bitcoin/bitcoin/issues/28133)
💬 MarcoFalke commented on pull request "doc: Clarify that -fstack-reuse=all bugs exist on all versions of GCC":
(https://github.com/bitcoin/bitcoin/pull/28105#issuecomment-1660617362)
It may be better to replace it with one that still reproduces. Though, there is an array of at least 5 of those bugs, presumably all due to the same underlying GCC bug. Instead of constantly updating the test case to the latest one that still reproduces, maybe just remove the outdated test case?
(https://github.com/bitcoin/bitcoin/pull/28105#issuecomment-1660617362)
It may be better to replace it with one that still reproduces. Though, there is an array of at least 5 of those bugs, presumably all due to the same underlying GCC bug. Instead of constantly updating the test case to the latest one that still reproduces, maybe just remove the outdated test case?
🚀 fanquake merged a pull request: "test: fix intermittent failure in p2p_getaddr_caching.py"
(https://github.com/bitcoin/bitcoin/pull/28144)
(https://github.com/bitcoin/bitcoin/pull/28144)
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280857120)
This used to be part of the default build, but now needs to be run manually: `make bitcoin-tidy-tests`
It's probably not a bad idea to have them run by c-i, that way we can see that they're actually catching something.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280857120)
This used to be part of the default build, but now needs to be run manually: `make bitcoin-tidy-tests`
It's probably not a bad idea to have them run by c-i, that way we can see that they're actually catching something.
📝 jonatack converted_to_draft a pull request: "test: update tool_wallet coverage for unexpected writes to wallet"
(https://github.com/bitcoin/bitcoin/pull/28116)
PR #15687 added test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet file, as described in issue https://github.com/bitcoin/bitcoin/issues/15608:
- wallet tool info unexpectedly writes to the wallet file if the wallet file permissions are read/write
- wallet tool info raises if the wallet file permissions are read-only
Update these tests/docs for a few changes since #15687:
- the addition of descriptor wallets
- the CI migration away from Appv
...
(https://github.com/bitcoin/bitcoin/pull/28116)
PR #15687 added test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet file, as described in issue https://github.com/bitcoin/bitcoin/issues/15608:
- wallet tool info unexpectedly writes to the wallet file if the wallet file permissions are read/write
- wallet tool info raises if the wallet file permissions are read-only
Update these tests/docs for a few changes since #15687:
- the addition of descriptor wallets
- the CI migration away from Appv
...
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660642848)
Thank you for looking into this! I will address trivial stuff/comments first and then will _try_ to address the other concerns in chronological order.
I am open to exploring in more detail the option of not putting the transaction in the mempool. I considered this in the beginning but I got the impression that this is going to be more complicated. Anyway I want to first try to address all concerns or, if not possible, get convinced that putting the transaction in the mempool is non-workable.
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660642848)
Thank you for looking into this! I will address trivial stuff/comments first and then will _try_ to address the other concerns in chronological order.
I am open to exploring in more detail the option of not putting the transaction in the mempool. I considered this in the beginning but I got the impression that this is going to be more complicated. Anyway I want to first try to address all concerns or, if not possible, get convinced that putting the transaction in the mempool is non-workable.
...
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1660646771)
I'm a little confused that there are no `NOLINT`s hooked up, and yet c-i is still passing. Beyond the one that @MarcoFalke pointed out above, surely more are still needed right?
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1660646771)
I'm a little confused that there are no `NOLINT`s hooked up, and yet c-i is still passing. Beyond the one that @MarcoFalke pointed out above, surely more are still needed right?
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280872672)
I intend to add a bunch of docs as a follow-up. I'll address these comments then as well.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280872672)
I intend to add a bunch of docs as a follow-up. I'll address these comments then as well.
💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660655910)
> 4th way
Currently the following is used:
* Bash as a driver for `./ci/lint/06_script.sh`, and in some lint script that weren't converted to python
* Python, for most lint scripts.
* Haskell (only if you compile shellcheck from source)
* pip dependencies (which may already use rust if they are wheels)
Personally I don't see a problem of adding a new dependency. Once and if this was merged, my plans were to at least remove the Bash driver, and fix the outstanding issues, e.g. to coll
...
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660655910)
> 4th way
Currently the following is used:
* Bash as a driver for `./ci/lint/06_script.sh`, and in some lint script that weren't converted to python
* Python, for most lint scripts.
* Haskell (only if you compile shellcheck from source)
* pip dependencies (which may already use rust if they are wheels)
Personally I don't see a problem of adding a new dependency. Once and if this was merged, my plans were to at least remove the Bash driver, and fix the outstanding issues, e.g. to coll
...
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280873822)
> It's probably not a bad idea to have them run by c-i, that way we can see that they're actually catching something.
Right, and also to ensure it compiles in the first place.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280873822)
> It's probably not a bad idea to have them run by c-i, that way we can see that they're actually catching something.
Right, and also to ensure it compiles in the first place.
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660675919)
> I considered this in the beginning but I got the impression that this is going to be more complicated.
My guess would have been that it will be a lot simpler, at least for reviewers.
> Edit: some parts of this PR will be reused even if changed to skip the mempool.
Yes, everything except for the mempool / high level p2p stuff can probably stay as-is.
> @MarcoFalke, yes, more concrete examples to try to shoot this down are welcome ;-)
Would be good to explain what is wrong/missing
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1660675919)
> I considered this in the beginning but I got the impression that this is going to be more complicated.
My guess would have been that it will be a lot simpler, at least for reviewers.
> Edit: some parts of this PR will be reused even if changed to skip the mempool.
Yes, everything except for the mempool / high level p2p stuff can probably stay as-is.
> @MarcoFalke, yes, more concrete examples to try to shoot this down are welcome ;-)
Would be good to explain what is wrong/missing
...
💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660676181)
> Currently the following is used:
Did you exclude clang-tidy here, or is it part of the list in the quote later in your comment. We already use it for linting, and my plan is to migrate more checks too it. For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check (LLVM 17 though).
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660676181)
> Currently the following is used:
Did you exclude clang-tidy here, or is it part of the list in the quote later in your comment. We already use it for linting, and my plan is to migrate more checks too it. For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check (LLVM 17 though).
💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660694977)
I think the CI clang-tidy task is completely separate from the CI lint task. Just to repeat some points: It requires a full build system, a compile_commands.json, and must do a full build (slow). For the specifics, it may use more complicated and more verbose code, and it apparently ignores commented code?
> For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check
Yes, this is possible, but comes with the above downsides. Or did I miss some
...
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1660694977)
I think the CI clang-tidy task is completely separate from the CI lint task. Just to repeat some points: It requires a full build system, a compile_commands.json, and must do a full build (slow). For the specifics, it may use more complicated and more verbose code, and it apparently ignores commented code?
> For example, we can replace our python based assert linter (and more) with the equivalent clang-tidy check
Yes, this is possible, but comes with the above downsides. Or did I miss some
...
💬 MarcoFalke commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1280896502)
I guess it may be simpler to serialize and deserialize the xor key as a raw byte span (without the size indicator). This should make it even easier to read in external scripts, for example in vanilla python.
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1280896502)
I guess it may be simpler to serialize and deserialize the xor key as a raw byte span (without the size indicator). This should make it even easier to read in external scripts, for example in vanilla python.
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280900182)
To clarify, I am trying to say:
```suggestion
LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); // NOLINT(bitcoin-unterminated-logprintf)
```
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1280900182)
To clarify, I am trying to say:
```suggestion
LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); // NOLINT(bitcoin-unterminated-logprintf)
```
💬 willcl-ark commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1660735316)
> [ @willcl-ark You could try to inspect the binary after compilation to see how the compiler optimized it (and what difference there is to your version) ]
I did take a rough look on my machine (and on [godbolt](https://godbolt.org/z/M9a7WG5s3)), but I don't think I have the necessary super-powers needed to really dissect why my (very naive and likely badly implemented) impl. didn't pay off.
From what I can see, it doesn't appear that clang16 is auto-SIMD-ing your version, but my SIMD vers
...
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1660735316)
> [ @willcl-ark You could try to inspect the binary after compilation to see how the compiler optimized it (and what difference there is to your version) ]
I did take a rough look on my machine (and on [godbolt](https://godbolt.org/z/M9a7WG5s3)), but I don't think I have the necessary super-powers needed to really dissect why my (very naive and likely badly implemented) impl. didn't pay off.
From what I can see, it doesn't appear that clang16 is auto-SIMD-ing your version, but my SIMD vers
...