💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276601095)
As you said, this one isn't a big deal but it does highlight the general issue of exceptions.
Generally I see a few ways to handle them:
- Change the code to be unambiguous. In this case it'd be something like adding a `LogPrintf_nonewline()` and using as necessary
- Hard-code exceptions in the plugin. This is basically how the current linter works.
- Try to find a preprocessor guard that applies only to clang-tidy. I can't find one, but I assume I'm just missing it.
- ^^, but define our
...
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276601095)
As you said, this one isn't a big deal but it does highlight the general issue of exceptions.
Generally I see a few ways to handle them:
- Change the code to be unambiguous. In this case it'd be something like adding a `LogPrintf_nonewline()` and using as necessary
- Hard-code exceptions in the plugin. This is basically how the current linter works.
- Try to find a preprocessor guard that applies only to clang-tidy. I can't find one, but I assume I'm just missing it.
- ^^, but define our
...
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1654072882)
The first issue is inside a container:
```
chattr: Operation not permitted while setting flags on /tmp/cirrus-build-3580104950/ci/scratch/test_runner/test_runner_₿_🏃_20230727_161727/feature_reindex_readonly_206/node0/regtest/blocks/blk00000.dat
```
The second issue is that the file can't be deleted, because you'll have to grant/undo the permission again before the end of the test.
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1654072882)
The first issue is inside a container:
```
chattr: Operation not permitted while setting flags on /tmp/cirrus-build-3580104950/ci/scratch/test_runner/test_runner_₿_🏃_20230727_161727/feature_reindex_readonly_206/node0/regtest/blocks/blk00000.dat
```
The second issue is that the file can't be deleted, because you'll have to grant/undo the permission again before the end of the test.
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1654074150)
https://duckduckgo.com/?q=chattr+inside+docker should fix it
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1654074150)
https://duckduckgo.com/?q=chattr+inside+docker should fix it
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276607112)
You could just add a `/* TIDY IGNORE LOG */` (or similar comment) in the source code and then skip those with the plugin?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276607112)
You could just add a `/* TIDY IGNORE LOG */` (or similar comment) in the source code and then skip those with the plugin?
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276610630)
> * Try to find a preprocessor guard that applies only to clang-tidy. I can't find one
> but I assume I'm just missing it.
Whoops, here it is: https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics
Neat. So the exceptions (if it works as documented) can become:
```
LogPrintf("foo..."); // NOLINT(bitcoin-unterminated-logprintf)
```
Which is nice and self-documenting :)
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276610630)
> * Try to find a preprocessor guard that applies only to clang-tidy. I can't find one
> but I assume I'm just missing it.
Whoops, here it is: https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics
Neat. So the exceptions (if it works as documented) can become:
```
LogPrintf("foo..."); // NOLINT(bitcoin-unterminated-logprintf)
```
Which is nice and self-documenting :)
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276618881)
Pushed a commit here which tests this: https://github.com/theuni/bitcoin-tidy-plugin/commit/cdc007022b6cd40320b7e74ab4cbb8e67a6d6e17
It works as expected.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276618881)
Pushed a commit here which tests this: https://github.com/theuni/bitcoin-tidy-plugin/commit/cdc007022b6cd40320b7e74ab4cbb8e67a6d6e17
It works as expected.
💬 achow101 commented on pull request "Fuzz: a more efficient descriptor parsing target":
(https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1654108231)
ACK 131314b62e899f95d1863083d303b489b3212b16
(https://github.com/bitcoin/bitcoin/pull/27888#issuecomment-1654108231)
ACK 131314b62e899f95d1863083d303b489b3212b16
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276624433)
> You could just add a `/* TIDY IGNORE LOG */` (or similar comment) in the source code and then skip those with the plugin?
Just to address this as I agree it'd be a reasonable solution, there's (currently at least) no access to comments from clang-tidy plugins as they're not part of the AST.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276624433)
> You could just add a `/* TIDY IGNORE LOG */` (or similar comment) in the source code and then skip those with the plugin?
Just to address this as I agree it'd be a reasonable solution, there's (currently at least) no access to comments from clang-tidy plugins as they're not part of the AST.
🚀 achow101 merged a pull request: "Fuzz: a more efficient descriptor parsing target"
(https://github.com/bitcoin/bitcoin/pull/27888)
(https://github.com/bitcoin/bitcoin/pull/27888)
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276667231)
Sorry for all the spam here, one more.
Given the nice opt-out above, I've pushed changes to my repo which eliminate all exceptions, so the plugin rule is now very simply: "every logprintf format string must end in a newline". That will mean we need to opt-out of the current "..." uses with NOLINT, which I think is the correct thing to do.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276667231)
Sorry for all the spam here, one more.
Given the nice opt-out above, I've pushed changes to my repo which eliminate all exceptions, so the plugin rule is now very simply: "every logprintf format string must end in a newline". That will mean we need to opt-out of the current "..." uses with NOLINT, which I think is the correct thing to do.
💬 jonatack commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#discussion_r1276671966)
That would encompass many of the methods in `rpc/util`. Where would you suggest to put them?
(https://github.com/bitcoin/bitcoin/pull/28134#discussion_r1276671966)
That would encompass many of the methods in `rpc/util`. Where would you suggest to put them?
💬 jonatack commented on pull request "rpc, util: deduplicate AmountFromValue() using util::Result":
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1654195359)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/28134#issuecomment-1654195359)
Rebased.
💬 luke-jr commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276717254)
Shouldn't we still execute the notification? While it's not possible for the client to confirm it worked, it should still execute...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276717254)
Shouldn't we still execute the notification? While it's not possible for the client to confirm it worked, it should still execute...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276726382)
My thinking was that since our RPC API does not have any notifications defined yet we could discard them for now. Like, our RPCs and responses are a separate category from notifications. But you have an interesting point, a user could send a notification with a method like `generatetoaddress` and just not want to wait for a reply. There would be plenty of "getter" RPCs that would make no sense in a notification however and just waste time on the server. Maybe this is best suited for a follow-up
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1276726382)
My thinking was that since our RPC API does not have any notifications defined yet we could discard them for now. Like, our RPCs and responses are a separate category from notifications. But you have an interesting point, a user could send a notification with a method like `generatetoaddress` and just not want to wait for a reply. There would be plenty of "getter" RPCs that would make no sense in a notification however and just waste time on the server. Maybe this is best suited for a follow-up
...
💬 achow101 commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654432367)
While I agree that doing something smart like this in the context of fee bumping is fine, `CreateTransaction` is a generic function for the wallet's transaction building so changing the behavior here will have an effect on other uses as well, so I don't think this is the right approach. In general, we shouldn't assume what the change is when it is ambiguous - conceivably a user could be reusing addresses or otherwise actually wants a particular change address that is also one of the outputs. Doi
...
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654432367)
While I agree that doing something smart like this in the context of fee bumping is fine, `CreateTransaction` is a generic function for the wallet's transaction building so changing the behavior here will have an effect on other uses as well, so I don't think this is the right approach. In general, we shouldn't assume what the change is when it is ambiguous - conceivably a user could be reusing addresses or otherwise actually wants a particular change address that is also one of the outputs. Doi
...
💬 murchandamus commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654451888)
Perhaps this is a good way forward: only merge the new change output and the existing output on the original transaction if either a) Bitcoin Core organically detects that the original output was the change and it is also provided as the custom change address again, or b) if the user explicitly instructs the wallet to treat the original transaction’s output as the change per https://github.com/bitcoin/bitcoin/pull/26467 and provides the same address as custom change again. Otherwise, if the outp
...
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1654451888)
Perhaps this is a good way forward: only merge the new change output and the existing output on the original transaction if either a) Bitcoin Core organically detects that the original output was the change and it is also provided as the custom change address again, or b) if the user explicitly instructs the wallet to treat the original transaction’s output as the change per https://github.com/bitcoin/bitcoin/pull/26467 and provides the same address as custom change again. Otherwise, if the outp
...
💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1654461507)
ACK 26f91a56afd01ce11245944c66361da9aaa6a71e
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1654461507)
ACK 26f91a56afd01ce11245944c66361da9aaa6a71e
📝 jonatack opened a pull request: "rpc, util: avoid string copies in ParseHash/ParseHex functions"
(https://github.com/bitcoin/bitcoin/pull/28172)
These utility methods are called by quite a few RPCs and tests, as well as by each other.
```
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
```
Also remove an out-of-date external link.
(https://github.com/bitcoin/bitcoin/pull/28172)
These utility methods are called by quite a few RPCs and tests, as well as by each other.
```
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
```
Also remove an out-of-date external link.
💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1276759711)
Perhaps check that this label doesn't appear in any of the migrated wallets?
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1276759711)
Perhaps check that this label doesn't appear in any of the migrated wallets?
💬 luke-jr commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1276765179)
Could just as well still be unsupported, not invalid.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1276765179)
Could just as well still be unsupported, not invalid.