💬 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
...
💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280886196)
In commit "RPC/Wallet: Convert walletprocesspsbt to use options parameter" (baf99a9c789779e89e7684a550e16cf1fb7ce862)
I think `OBJ` should be replaced `OBJ_NAMED_PARAMS` so generated documentation is consistent with other RPC methods that take options parameters.
Replacing `OBJ` with `OBJ_NAMED_PARAMS` will also allow `sighashtype` and `bip32derivs` and `finalize` options to be continue to be accepted as named parameters if the backwards compatibility code below accepting them as positiona
...
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280886196)
In commit "RPC/Wallet: Convert walletprocesspsbt to use options parameter" (baf99a9c789779e89e7684a550e16cf1fb7ce862)
I think `OBJ` should be replaced `OBJ_NAMED_PARAMS` so generated documentation is consistent with other RPC methods that take options parameters.
Replacing `OBJ` with `OBJ_NAMED_PARAMS` will also allow `sighashtype` and `bip32derivs` and `finalize` options to be continue to be accepted as named parameters if the backwards compatibility code below accepting them as positiona
...
🤔 ryanofsky reviewed a pull request: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-1557402115)
Light code review ACK baf99a9c789779e89e7684a550e16cf1fb7ce862
(https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-1557402115)
Light code review ACK baf99a9c789779e89e7684a550e16cf1fb7ce862
💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280895729)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)
I think this `arg.m_type` reference on line 730 also needs to be replaced by `argtype`.
If we got rid of the `RPCArg::m_type` member (see other comment) it would avoid this type of bug.
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280895729)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)
I think this `arg.m_type` reference on line 730 also needs to be replaced by `argtype`.
If we got rid of the `RPCArg::m_type` member (see other comment) it would avoid this type of bug.
💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280898093)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)
I think just calling this new struct member `m_types` would be more consistent with the `m_names` member. Also I think a followup commit should remove the `m_type` member which is now confusing and redundant.
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280898093)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)
I think just calling this new struct member `m_types` would be more consistent with the `m_names` member. Also I think a followup commit should remove the `m_type` member which is now confusing and redundant.
💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280912926)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)
It would be good to have unit test coverage for this new code. Should be easy to add a test creating an RPCArg and calling this method in rpc_tests.cpp.
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1280912926)
In commit "RPC: Support specifying different types for param aliases" (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)
It would be good to have unit test coverage for this new code. Should be easy to add a test creating an RPCArg and calling this method in rpc_tests.cpp.
👍 Sjors approved a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1557474340)
re-ACK e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1557474340)
re-ACK e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518
🤔 stickies-v reviewed a pull request: "kernel: Prune leveldb headers"
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1556786030)
Concept ACK, did a first run-through of the code but will dig into it deeper.
(https://github.com/bitcoin/bitcoin/pull/28186#pullrequestreview-1556786030)
Concept ACK, did a first run-through of the code but will dig into it deeper.
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280862595)
nit: `const DataStream& ssKey`?
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280862595)
nit: `const DataStream& ssKey`?
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280723077)
`WriteImpl`, `ssKey` and `ssValue` are all `CDBBatch` class members, I'm not sure passing these as parameters makes sense? And if so, I think `ssKey` should be `const`.
At first sight, I'm not entirely sure why `ssKey` and `ssValue` are `CDBBatch` members when they get resized and cleared in every operation, so perhaps cleaning that up is the better way to go, but I don't like the ambiguity the current implementation introduces.
And this comment applies pretty much identically to 5c9e87fcd
...
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280723077)
`WriteImpl`, `ssKey` and `ssValue` are all `CDBBatch` class members, I'm not sure passing these as parameters makes sense? And if so, I think `ssKey` should be `const`.
At first sight, I'm not entirely sure why `ssKey` and `ssValue` are `CDBBatch` members when they get resized and cleared in every operation, so perhaps cleaning that up is the better way to go, but I don't like the ambiguity the current implementation introduces.
And this comment applies pretty much identically to 5c9e87fcd
...
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280498519)
nit: `const std::string&`?
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280498519)
nit: `const std::string&`?
💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280943436)
nit: `const DataStream& ssKey`?
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1280943436)
nit: `const DataStream& ssKey`?
💬 ryanofsky commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280950527)
In commit "wallet: return and display signer error" (0e0163a062b09a3f762d7dd0af3d6e78e675aae1)
s/translated/original/ here since RPC methods return untranslated error strings
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1280950527)
In commit "wallet: return and display signer error" (0e0163a062b09a3f762d7dd0af3d6e78e675aae1)
s/translated/original/ here since RPC methods return untranslated error strings