💬 dergoegge commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235821583)
Could you elaborate what exactly is making this input so slow? I doubt it's the actual json parsing. It looks more like another descriptor parsing "bug".
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235821583)
Could you elaborate what exactly is making this input so slow? I doubt it's the actual json parsing. It looks more like another descriptor parsing "bug".
💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235826082)
Correct, the workaround for the descriptor parsing are not applied here. So an alternative would be to apply them here.
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235826082)
Correct, the workaround for the descriptor parsing are not applied here. So an alternative would be to apply them here.
💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235830888)
With "workaround" I mean https://github.com/bitcoin/bitcoin/pull/30197
(https://github.com/bitcoin/bitcoin/pull/30473#issuecomment-2235830888)
With "workaround" I mean https://github.com/bitcoin/bitcoin/pull/30197
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682359897)
Reset back to prior version. As you say it seems we could convert `uint256S()` to a constructor already thanks to the pre-C++23 warnings by Clang & GCC, but I think that is outside the scope of this PR.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682359897)
Reset back to prior version. As you say it seems we could convert `uint256S()` to a constructor already thanks to the pre-C++23 warnings by Clang & GCC, but I think that is outside the scope of this PR.
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682361588)
Reset back to prior version, see https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682361588)
Reset back to prior version, see https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682274787.
💬 dergoegge commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682362271)
Perhaps a more descriptive comment would be good, e.g.: `We don't call PickValue on the outpoints set directly because PickValue will advance a begin iterator on the set, which is expensive, because it only has a "++" operator. As it would be called in a loop of num_in (~outpoints.size()), the runtime would be O(outpoints.size() ^ 2). Instead we create a std::vector copy of the set, which allows for O(1) std::advance.`.
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682362271)
Perhaps a more descriptive comment would be good, e.g.: `We don't call PickValue on the outpoints set directly because PickValue will advance a begin iterator on the set, which is expensive, because it only has a "++" operator. As it would be called in a loop of num_in (~outpoints.size()), the runtime would be O(outpoints.size() ^ 2). Instead we create a std::vector copy of the set, which allows for O(1) std::advance.`.
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2235847045)
I forgot to mention the fuzz input for testing. It is `c6efad2b3a41cf2a2a682dabed1310bf5e3c101e`. See https://cirrus-ci.com/task/6178647134961664?logs=ci#L4058
```
Run txorphan with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/txorphan')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3180047797
INFO: Loaded 1 modules (623498 inline 8-bit cou
...
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2235847045)
I forgot to mention the fuzz input for testing. It is `c6efad2b3a41cf2a2a682dabed1310bf5e3c101e`. See https://cirrus-ci.com/task/6178647134961664?logs=ci#L4058
```
Run txorphan with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/txorphan')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3180047797
INFO: Loaded 1 modules (623498 inline 8-bit cou
...
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682373792)
Thanks, done!
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1682373792)
Thanks, done!
✅ maflcko closed an issue: "fuzz: Fix timeouts"
(https://github.com/bitcoin/bitcoin/issues/28812)
(https://github.com/bitcoin/bitcoin/issues/28812)
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2235880636)
Closing for now. It is probably better to create separate issues for each fuzz target, going forward.
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2235880636)
Closing for now. It is probably better to create separate issues for each fuzz target, going forward.
💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2235889412)
> Would it not be possible to implement this by creating a wallet called broadcast_pool and then write simple wrapper functions to implement the broadcast pool features?
All of the wallets linked in the issues above (and of course many others) use the Electrum server protocol, and in general rely on Electrum server implementations that do not require or use the wallet functionality. Even if they were to do so, I cannot see how it is practical to broadcast arbitrary transactions via the wallet
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2235889412)
> Would it not be possible to implement this by creating a wallet called broadcast_pool and then write simple wrapper functions to implement the broadcast pool features?
All of the wallets linked in the issues above (and of course many others) use the Electrum server protocol, and in general rely on Electrum server implementations that do not require or use the wallet functionality. Even if they were to do so, I cannot see how it is practical to broadcast arbitrary transactions via the wallet
...
⚠️ maflcko opened an issue: "scriptpubkeyman fuzz target TopUp is slow"
(https://github.com/bitcoin/bitcoin/issues/30476)
Seems wasteful to spend time in `TopUp` while fuzzing, so my recommendation would be to limit TopUp iterations a bit.
Context: https://cirrus-ci.com/task/6178647134961664?logs=ci#L4357
```
Run scriptpubkeyman with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/scriptpubkeyman')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3840853548
INFO: Loa
...
(https://github.com/bitcoin/bitcoin/issues/30476)
Seems wasteful to spend time in `TopUp` while fuzzing, so my recommendation would be to limit TopUp iterations a bit.
Context: https://cirrus-ci.com/task/6178647134961664?logs=ci#L4357
```
Run scriptpubkeyman with args ['/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/scriptpubkeyman')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3840853548
INFO: Loa
...
👍 dergoegge approved a pull request: "fuzz: Speed up PickValue in txorphan"
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2185052729)
utACK fa4e7964792255a34df4813c834aaa2f8ca53e46
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2185052729)
utACK fa4e7964792255a34df4813c834aaa2f8ca53e46
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1682438692)
We can just drop this for now.
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1682438692)
We can just drop this for now.
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2235940270)
> I suggest passing -DCMAKE_BUILD_TYPE=None to disable this overriding, which also works with DEBUG=1.
I'm not exactly sure what we want to do with build types and CMake packages in depends going forward, but have used `None` here for now.
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2235940270)
> I suggest passing -DCMAKE_BUILD_TYPE=None to disable this overriding, which also works with DEBUG=1.
I'm not exactly sure what we want to do with build types and CMake packages in depends going forward, but have used `None` here for now.
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2235951152)
I ran into:
```
make -j17 src/bitcoin-mine
make: *** No rule to make target `src/bitcoin-mine'. Stop.
```
Fixed with https://github.com/Sjors/bitcoin/commit/68979b8f4c4e069bc9b6d19679ef6832839ceae4
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2235951152)
I ran into:
```
make -j17 src/bitcoin-mine
make: *** No rule to make target `src/bitcoin-mine'. Stop.
```
Fixed with https://github.com/Sjors/bitcoin/commit/68979b8f4c4e069bc9b6d19679ef6832839ceae4
💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236001712)
> > If you really care about putting the transaction in the mempool, you can use prioritisetransaction.
>
> My understanding from reading the original PR is that this affects inclusion of transactions into a proposed block, and not retention in the mempool. As you suggest though, changing the contents of the mempool for this purpose is not ideal.
While the RPC is placed in the "mining" section, it also affects the mempool size trimming calculation. But yeah, using the RPC here may not be i
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236001712)
> > If you really care about putting the transaction in the mempool, you can use prioritisetransaction.
>
> My understanding from reading the original PR is that this affects inclusion of transactions into a proposed block, and not retention in the mempool. As you suggest though, changing the contents of the mempool for this purpose is not ideal.
While the RPC is placed in the "mining" section, it also affects the mempool size trimming calculation. But yeah, using the RPC here may not be i
...
💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2236033690)
Re-running known Wine CI failure.
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2236033690)
Re-running known Wine CI failure.
💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236090225)
> Do you need the views to determine which transactions are "active", for calculating the wallet balance and seeing which coins are spendable in the view?
Yes - the wallet would consider any UTXOs included in these transactions as spent, and any UTXOs they create would be spendable. Of course there would need to be consideration in a wallet UI to avoid creating a situation where non-existent UTXOs are spent in a broadcasted transaction (which would simply fail on broadcast as it does now).
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236090225)
> Do you need the views to determine which transactions are "active", for calculating the wallet balance and seeing which coins are spendable in the view?
Yes - the wallet would consider any UTXOs included in these transactions as spent, and any UTXOs they create would be spendable. Of course there would need to be consideration in a wallet UI to avoid creating a situation where non-existent UTXOs are spent in a broadcasted transaction (which would simply fail on broadcast as it does now).
...
💬 maflcko commented on pull request "fuzz: add target for `CoinsResult`":
(https://github.com/bitcoin/bitcoin/pull/30461#issuecomment-2236115380)
Not sure what the point here is. All functions are trivial for-loops, and already fuzzed.
The only function that isn't `CoinsResult::Erase` is unused outside of tests, so I don't see why CPU, storage, code, review, maintenance, etc should be spent on this.
It may be better to remove the function or move it to the tests?
(https://github.com/bitcoin/bitcoin/pull/30461#issuecomment-2236115380)
Not sure what the point here is. All functions are trivial for-loops, and already fuzzed.
The only function that isn't `CoinsResult::Erase` is unused outside of tests, so I don't see why CPU, storage, code, review, maintenance, etc should be spent on this.
It may be better to remove the function or move it to the tests?