💬 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.
💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236188801)
> 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).
I wonder if the feature helps users in that case. If a set of transactions isn't in "the mempool" (the one the wallet is connected to) right now, it will p
...
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2236188801)
> 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).
I wonder if the feature helps users in that case. If a set of transactions isn't in "the mempool" (the one the wallet is connected to) right now, it will p
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682577501)
Now that we have this, could you please cover it with tests as well (where we're testing that the linked list is constructed correctly)?
Or would it make more sense to just call `SanityCheck()`since it's already checking the linked list's integrity?
<img src="https://github.com/user-attachments/assets/782fc1a0-8d78-4ad2-a963-8e77337a8a39">
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682577501)
Now that we have this, could you please cover it with tests as well (where we're testing that the linked list is constructed correctly)?
Or would it make more sense to just call `SanityCheck()`since it's already checking the linked list's integrity?
<img src="https://github.com/user-attachments/assets/782fc1a0-8d78-4ad2-a963-8e77337a8a39">
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682588039)
This is only called in the [fuzz tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coinscache_sim.cpp), right?
Is it just bad luck that it wasn't triggered by CI or could the fuzz tests be incorrect or just not yet triggered at this stage?
<img width='400px' src="https://github.com/user-attachments/assets/196ed1a1-7c2c-4181-b0e6-d19eaf789e4a">
Could we use it in `coins_tests` or `coinscachepair_tests` as well?
And if it's a test-only method, maybe document, that it's h
...
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682588039)
This is only called in the [fuzz tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/coinscache_sim.cpp), right?
Is it just bad luck that it wasn't triggered by CI or could the fuzz tests be incorrect or just not yet triggered at this stage?
<img width='400px' src="https://github.com/user-attachments/assets/196ed1a1-7c2c-4181-b0e6-d19eaf789e4a">
Could we use it in `coins_tests` or `coinscachepair_tests` as well?
And if it's a test-only method, maybe document, that it's h
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682615656)
Any reason not returning directly like in `Begin()`?
```suggestion
inline CoinsCachePair* Next(CoinsCachePair& current) noexcept { return current.second.Next(); }
```
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682615656)
Any reason not returning directly like in `Begin()`?
```suggestion
inline CoinsCachePair* Next(CoinsCachePair& current) noexcept { return current.second.Next(); }
```
💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682629622)
Fuzz coverage isn't covered by corecheck. It is produced daily at https://maflcko.github.io/b-c-cov/fuzz.coverage/src/coins.cpp.gcov.html (for master).
It can also be produced (manually triggered) for this pull request, if you think it would be useful.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682629622)
Fuzz coverage isn't covered by corecheck. It is produced daily at https://maflcko.github.io/b-c-cov/fuzz.coverage/src/coins.cpp.gcov.html (for master).
It can also be produced (manually triggered) for this pull request, if you think it would be useful.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682638514)
Thanks for the info.
I assumed that some of the randomized tests contributed to the coverage here. Isn't that why we have `Lost baseline coverage` for PRs? Or did I misunderstand the reason for unrelated code not being covered anymore?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682638514)
Thanks for the info.
I assumed that some of the randomized tests contributed to the coverage here. Isn't that why we have `Lost baseline coverage` for PRs? Or did I misunderstand the reason for unrelated code not being covered anymore?
💬 maflcko commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682651787)
> I assumed that some of the randomized tests contributed to the coverage here.
The section where your screenshot is from is "Uncovered new code". The screenshot means that the code wasn't covered previously and isn't covered now either. There should be no randomness.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682651787)
> I assumed that some of the randomized tests contributed to the coverage here.
The section where your screenshot is from is "Uncovered new code". The screenshot means that the code wasn't covered previously and isn't covered now either. There should be no randomness.
💬 glozow commented on issue "Change estimate_mode default to "ECONOMICAL" in these RPC calls":
(https://github.com/bitcoin/bitcoin/issues/30009#issuecomment-2236230361)
> This code suggests it does, but I haven't tested:
@Sjors that code is for wallet getting an estimate when it's making a BIP125-signaling tx.
People on this thread may want to review #30275
(https://github.com/bitcoin/bitcoin/issues/30009#issuecomment-2236230361)
> This code suggests it does, but I haven't tested:
@Sjors that code is for wallet getting an estimate when it's making a BIP125-signaling tx.
People on this thread may want to review #30275
📝 hebasto opened a pull request: "depends: Amend handling flags environment variables"
(https://github.com/bitcoin/bitcoin/pull/30477)
The `{C,CXX,CPP,LD}FLAGS` are build type-agnostic. Therefore, if any of them is specified, it should be assigned to a non-type-specific variable.
This PR is split from https://github.com/bitcoin/bitcoin/pull/30454. It is required because CMake distinguishes [build type-specific variables](https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_CONFIG.html).
No behaviour change for Autotools:
- on the master branch @ efbf4e71ce8e3cd49ccdfb5e55e14fa4b338453c
```
$ make -C depends pr
...
(https://github.com/bitcoin/bitcoin/pull/30477)
The `{C,CXX,CPP,LD}FLAGS` are build type-agnostic. Therefore, if any of them is specified, it should be assigned to a non-type-specific variable.
This PR is split from https://github.com/bitcoin/bitcoin/pull/30454. It is required because CMake distinguishes [build type-specific variables](https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_CONFIG.html).
No behaviour change for Autotools:
- on the master branch @ efbf4e71ce8e3cd49ccdfb5e55e14fa4b338453c
```
$ make -C depends pr
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682658330)
yes, of course.
I assumed that the existence of `Lost baseline coverage` means that there was some randomness in the tests or infrastructure or production code or threading or something (i.e. covered before, but not covered now for a reason that's unrelated to this change):
<img width="400" alt="image" src="https://github.com/user-attachments/assets/b319ec1f-cde8-4a83-9e17-8e5949af0cf5">
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1682658330)
yes, of course.
I assumed that the existence of `Lost baseline coverage` means that there was some randomness in the tests or infrastructure or production code or threading or something (i.e. covered before, but not covered now for a reason that's unrelated to this change):
<img width="400" alt="image" src="https://github.com/user-attachments/assets/b319ec1f-cde8-4a83-9e17-8e5949af0cf5">