💬 jonatack commented on pull request "test, rpc: invalid sighashtype coverage":
(https://github.com/bitcoin/bitcoin/pull/28166#issuecomment-1653588511)
Thanks @MarcoFalke, done.
(https://github.com/bitcoin/bitcoin/pull/28166#issuecomment-1653588511)
Thanks @MarcoFalke, done.
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387)
Not sure how effective it is to put the transaction into the mempool. I know that your implementation treats the positive case (reply to an `inv` as if the tx wasn't there, or withhold the tx in reply to a `getdata`). However, a failure to reply to an otherwise valid `inv` can still be used to query the mempool. Or similarly, a failure to (also) withhold related transactions in an otherwise valid `getdata` can be used to query the mempool. (For example, if the transaction was a CPFP and the pare
...
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387)
Not sure how effective it is to put the transaction into the mempool. I know that your implementation treats the positive case (reply to an `inv` as if the tx wasn't there, or withhold the tx in reply to a `getdata`). However, a failure to reply to an otherwise valid `inv` can still be used to query the mempool. Or similarly, a failure to (also) withhold related transactions in an otherwise valid `getdata` can be used to query the mempool. (For example, if the transaction was a CPFP and the pare
...
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653599491)
> Should this also remove the /* Continued */ code-bloat?
Will add this.
Back to draft while we look at an in-tree approach.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653599491)
> Should this also remove the /* Continued */ code-bloat?
Will add this.
Back to draft while we look at an in-tree approach.
📝 fanquake converted_to_draft a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)
Demo of integrating the bitcoin-tidy (https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job. The plugin currently has a single check, `bitcoin-unterminated-logprintf`, the purpose of which would be to replace our Python based lint-logs linter.
The check currently produces output on master, i.e:
```bash
clang-tidy-16 -p=/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -lo
...
(https://github.com/bitcoin/bitcoin/pull/26296)
Demo of integrating the bitcoin-tidy (https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job. The plugin currently has a single check, `bitcoin-unterminated-logprintf`, the purpose of which would be to replace our Python based lint-logs linter.
The check currently produces output on master, i.e:
```bash
clang-tidy-16 -p=/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -lo
...
💬 MarcoFalke commented on pull request "test, rpc: invalid sighashtype coverage":
(https://github.com/bitcoin/bitcoin/pull/28166#discussion_r1276267442)
unrelated: Just for reference, `31.99999999` is *really* `31.99999998999999917259629000909626483917236328125`
(https://github.com/bitcoin/bitcoin/pull/28166#discussion_r1276267442)
unrelated: Just for reference, `31.99999999` is *really* `31.99999998999999917259629000909626483917236328125`
👋 MarcoFalke's pull request is ready for review: "refactor: Remove unused raw-pointer read helper from univalue"
(https://github.com/bitcoin/bitcoin/pull/28168)
(https://github.com/bitcoin/bitcoin/pull/28168)
💬 MarcoFalke commented on pull request "refactor: Remove unused raw-pointer read helper from univalue":
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276269972)
Thanks, no idea if and why it worked, but the Windows CI passed the step.
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276269972)
Thanks, no idea if and why it worked, but the Windows CI passed the step.
👍 stickies-v approved a pull request: "refactor: Remove unused raw-pointer read helper from univalue"
(https://github.com/bitcoin/bitcoin/pull/28168#pullrequestreview-1549833812)
utACK fa940f41eaffa4b2a28c465a10a4c12d4b8976b8
(https://github.com/bitcoin/bitcoin/pull/28168#pullrequestreview-1549833812)
utACK fa940f41eaffa4b2a28c465a10a4c12d4b8976b8
💬 MarcoFalke commented on pull request "refactor: Remove unused raw-pointer read helper from univalue":
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276271178)
> can't get iwyu to work on this file
You can just use the output produced by CI (tidy)
> but i think #include <cstring> is now no longer required
Thanks, done.
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276271178)
> can't get iwyu to work on this file
You can just use the output produced by CI (tidy)
> but i think #include <cstring> is now no longer required
Thanks, done.
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653641372)
Heh, really sorry @fanquake .
I think the out-of-tree approach makes sense for (for example) [plugins where we can create attributes and assign them meaning](https://github.com/theuni/bitcoin-core-clang-plugin/commit/05fb23f21df32df4445951123d281172b12ee61b). In that case, the "atomicity" issue goes away, because the code either respects the attribute or it doesn't.
But yeah, now that @MarcoFalke mentions it, that approach is cumbersome when we have to worry about things getting out of syn
...
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653641372)
Heh, really sorry @fanquake .
I think the out-of-tree approach makes sense for (for example) [plugins where we can create attributes and assign them meaning](https://github.com/theuni/bitcoin-core-clang-plugin/commit/05fb23f21df32df4445951123d281172b12ee61b). In that case, the "atomicity" issue goes away, because the code either respects the attribute or it doesn't.
But yeah, now that @MarcoFalke mentions it, that approach is cumbersome when we have to worry about things getting out of syn
...
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653647053)
> @fanquake Want to keep going here, or shall I open a new PR?
I can convert this over. I think retaining the current build as-is, is ok.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653647053)
> @fanquake Want to keep going here, or shall I open a new PR?
I can convert this over. I think retaining the current build as-is, is ok.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276302188)
Yeah, it does not provide a direct access to an arbitrary element. This is `O(unbroadcast_txids.size())` if faster is required we have to change the container (or extend it with supplementary vector). I shortened this a bit to:
```cpp
const auto tx_iter = std::next(unbroadcast_txids.begin(), FastRandomContext{}.randrange(unbroadcast_txids.size()));
```
This is still linear, but at least a bit short and (I hope) as easy to read.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276302188)
Yeah, it does not provide a direct access to an arbitrary element. This is `O(unbroadcast_txids.size())` if faster is required we have to change the container (or extend it with supplementary vector). I shortened this a bit to:
```cpp
const auto tx_iter = std::next(unbroadcast_txids.begin(), FastRandomContext{}.randrange(unbroadcast_txids.size()));
```
This is still linear, but at least a bit short and (I hope) as easy to read.
💬 ajtowns commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653739351)
Approach ACK. Seems like a fine idea to me.
> What is the rationale for encoding as text rather than bytes? SQLite can store byte values as BLOBs.
It's a python conversion script: can't you just add a command-line option for the resulting db to have hex txids or big/little endian blobs if there's user demand for it? Hex encoding seems a fine default to me, for what it's worth.
If people end up wanting lots of different options (convert scriptPubKeys to addresses? some way to update the
...
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653739351)
Approach ACK. Seems like a fine idea to me.
> What is the rationale for encoding as text rather than bytes? SQLite can store byte values as BLOBs.
It's a python conversion script: can't you just add a command-line option for the resulting db to have hex txids or big/little endian blobs if there's user demand for it? Hex encoding seems a fine default to me, for what it's worth.
If people end up wanting lots of different options (convert scriptPubKeys to addresses? some way to update the
...
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653740441)
Reworked. I'm guessing some of our linters could fail against the new lint code? If so, I think we should just exclude it from them. Added some more fixups. Updated the PR description. Not 100% sure about the CI (build) dir handling, but this can also be improved.
Added a commit to drop the `/* Continued */` markers. Can be squashed into the previous commit which drops lint-logs.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653740441)
Reworked. I'm guessing some of our linters could fail against the new lint code? If so, I think we should just exclude it from them. Added some more fixups. Updated the PR description. Not 100% sure about the CI (build) dir handling, but this can also be improved.
Added a commit to drop the `/* Continued */` markers. Can be squashed into the previous commit which drops lint-logs.
💬 jamesob commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653747991)
Concept ACK, will test soon
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653747991)
Concept ACK, will test soon
💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1653766453)
Maybe a release note would be nice?
Not because this would break anything, but so that users who are currently performing active measures to stay connected to multiple networks (e.g. `-addnode` together with regular monitoring if the added node is still online) know that there is another option now.
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1653766453)
Maybe a release note would be nice?
Not because this would break anything, but so that users who are currently performing active measures to stay connected to multiple networks (e.g. `-addnode` together with regular monitoring if the added node is still online) know that there is another option now.
💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276382028)
> I am also thinking of adding a util::Messages{Result&&} helper.
That sounds like a worthwhile improvement of the ergonomics here.
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276382028)
> I am also thinking of adding a util::Messages{Result&&} helper.
That sounds like a worthwhile improvement of the ergonomics here.
💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276417417)
It strikes me as unfortunate that the move constructor cannot not move the failure values across different result value types, meaning the failure needs to be passed in as a separate argument. At the same time the user would be allowed to return a `{std::move(result), util::Error{Untranslated("str error")}`, potentially (and I am still not quite sure if that is what is actually happening here) without the user noticing that this will not move the failure value and instead initialize a `Monostate
...
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276417417)
It strikes me as unfortunate that the move constructor cannot not move the failure values across different result value types, meaning the failure needs to be passed in as a separate argument. At the same time the user would be allowed to return a `{std::move(result), util::Error{Untranslated("str error")}`, potentially (and I am still not quite sure if that is what is actually happening here) without the user noticing that this will not move the failure value and instead initialize a `Monostate
...
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276418105)
Run clang-format on new code?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276418105)
Run clang-format on new code?
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276421094)
I guess the build is fast and not needed to be cached for now?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276421094)
I guess the build is fast and not needed to be cached for now?