💬 jonatack commented on pull request "test, rpc: invalid sighashtype coverage":
(https://github.com/bitcoin/bitcoin/pull/28166#discussion_r1276252322)
Done
(https://github.com/bitcoin/bitcoin/pull/28166#discussion_r1276252322)
Done
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276249120)
nit: Any reason to re-implement `std::advance()`?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276249120)
nit: Any reason to re-implement `std::advance()`?
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276243984)
shouldn't this use the peerman options struct?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276243984)
shouldn't this use the peerman options struct?
💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276240357)
Not sure if the low level net layer is the right place to inspect high level p2p messages and then decide to drop them. It may be good to enumerate why this is needed (if at all).
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276240357)
Not sure if the low level net layer is the right place to inspect high level p2p messages and then decide to drop them. It may be good to enumerate why this is needed (if at all).
💬 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.