💬 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?
💬 itornaza commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1682584021)
I am also wandering how the assignment involving the default constructor will clear the existing keypair data from memory, but I might be naive as well!
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1682584021)
I am also wandering how the assignment involving the default constructor will clear the existing keypair data from memory, but I might be naive as well!
✅ brunoerg closed a pull request: "fuzz: add target for `CoinsResult`"
(https://github.com/bitcoin/bitcoin/pull/30461)
(https://github.com/bitcoin/bitcoin/pull/30461)
💬 brunoerg commented on pull request "fuzz: add target for `CoinsResult`":
(https://github.com/bitcoin/bitcoin/pull/30461#issuecomment-2236140598)
> It may be better to remove the function or move it to the tests?
Probably move it to the tests. Anyway, I'll close this for now.
(https://github.com/bitcoin/bitcoin/pull/30461#issuecomment-2236140598)
> It may be better to remove the function or move it to the tests?
Probably move it to the tests. Anyway, I'll close this for now.
💬 itornaza commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1682594351)
It would be trivial to also erase the `keypair` even if it turns out not to be valid, or would that be paranoid?
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1682594351)
It would be trivial to also erase the `keypair` even if it turns out not to be valid, or would that be paranoid?
💬 glozow commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1682618237)
Maybe for this PR, it would be cleaner to drop the first commit and just change the default, so we can focus the review on the setting instead of the help text?
2ee3d774815f5e1840cb37d2a92007f19c217e4c seems helpful but can be a separate PR.
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1682618237)
Maybe for this PR, it would be cleaner to drop the first commit and just change the default, so we can focus the review on the setting instead of the help text?
2ee3d774815f5e1840cb37d2a92007f19c217e4c seems helpful but can be a separate PR.
💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682619292)
Thanks for your comprehensive input here @maflcko. You're right that this is safe as long as one CI job fails, there's no need to ensure this fails on every single compiler, which indeed makes the fear historical. My apologies for the back and forth.
Your suggestion for a `FromHex()` method also seems better than a `std::string_view` ctor.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682619292)
Thanks for your comprehensive input here @maflcko. You're right that this is safe as long as one CI job fails, there's no need to ensure this fails on every single compiler, which indeed makes the fear historical. My apologies for the back and forth.
Your suggestion for a `FromHex()` method also seems better than a `std::string_view` ctor.