💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1847953343)
Rebased and fixed encoding mistake I made during rebase:
```diff
diff --git a/src/node/sv2_messages.h b/src/node/sv2_messages.h
index fd6974b998..d3b257c31c 100644
--- a/src/node/sv2_messages.h
+++ b/src/node/sv2_messages.h
@@ -533,11 +533,11 @@ struct Sv2SubmitSolutionMsg
s >> m_template_id >> m_version >> m_header_timestamp >> m_header_nonce;
// Ignore the 2 byte length as the rest of the stream is assumed to be
// the m_coinbase_tx.
s.ignore(
...
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1847953343)
Rebased and fixed encoding mistake I made during rebase:
```diff
diff --git a/src/node/sv2_messages.h b/src/node/sv2_messages.h
index fd6974b998..d3b257c31c 100644
--- a/src/node/sv2_messages.h
+++ b/src/node/sv2_messages.h
@@ -533,11 +533,11 @@ struct Sv2SubmitSolutionMsg
s >> m_template_id >> m_version >> m_header_timestamp >> m_header_nonce;
// Ignore the 2 byte length as the rest of the stream is assumed to be
// the m_coinbase_tx.
s.ignore(
...
💬 sipsorcery commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1847971114)
It's something to do with `CMainSignals&`. Wherever it's used in libbitcoin_qt the msvc compiler generates a syntax error of:
`Error C2238 unexpected token(s) preceding ';'`
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1847971114)
It's something to do with `CMainSignals&`. Wherever it's used in libbitcoin_qt the msvc compiler generates a syntax error of:
`Error C2238 unexpected token(s) preceding ';'`
💬 kashifs commented on pull request "Make bitcoin-tx replaceable value optional":
(https://github.com/bitcoin/bitcoin/pull/29022#discussion_r1421163198)
Hi @pinheadmz,
Thanks for taking the time to review this. I've updated the Makefile.
For the documentation, I wanted to make it clear to the end user that if a transaction with no inputs called `tx_noinputs_replaceable` is created by running the following command:`./bitcoin-tx -create replaceable`, it should NOT be assumed that inputs later added to `tx_noinputs_replaceable` will be replaceable.
Is this already obvious? If not, is there other language that I should use to make this clear?
...
(https://github.com/bitcoin/bitcoin/pull/29022#discussion_r1421163198)
Hi @pinheadmz,
Thanks for taking the time to review this. I've updated the Makefile.
For the documentation, I wanted to make it clear to the end user that if a transaction with no inputs called `tx_noinputs_replaceable` is created by running the following command:`./bitcoin-tx -create replaceable`, it should NOT be assumed that inputs later added to `tx_noinputs_replaceable` will be replaceable.
Is this already obvious? If not, is there other language that I should use to make this clear?
...
💬 achow101 commented on issue "`-min` does not minimize wallet loading dialog":
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1848033095)
Was able replicate in 26.0, needed to use the release built binaries. I guess something in the way we build qt is causing this?
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1848033095)
Was able replicate in 26.0, needed to use the release built binaries. I guess something in the way we build qt is causing this?
💬 kevkevinpal commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#discussion_r1421212317)
nit: To me it confused me for a moment cause I didn't realize that f was using the operator defined on the line above, I think it would make more sense for a reader to just see the same thing above defined backwards like so
`friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(f.nSatoshisPerK * a); }`
feel free to ignore this suggestion though
(https://github.com/bitcoin/bitcoin/pull/29037#discussion_r1421212317)
nit: To me it confused me for a moment cause I didn't realize that f was using the operator defined on the line above, I think it would make more sense for a reader to just see the same thing above defined backwards like so
`friend CFeeRate operator*(int a, const CFeeRate& f) { return CFeeRate(f.nSatoshisPerK * a); }`
feel free to ignore this suggestion though
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1848150890)
Scenario: bustabit-tiny-hot-wallet_6h-YTD-2023
| Scenario File | Current Balance | Mean #UTXO | Current #UTXO | #Deposits | #Inputs Spent | #Withdraws | #Uneconomical outputs spent | #Change Created | #Changeless | Min Change Value | Max Change Value | Mean Change Value | Std. Dev. of Change Value | Total Fees | Mean Fees per Withdraw | Cost to Empty (10 sat/vB) | Total Cost | Min Input Size | Max Input Size | Mean Input Size | Std. Dev. of Input Size | Usage |
| --- | --- | --- | --- | ---
...
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1848150890)
Scenario: bustabit-tiny-hot-wallet_6h-YTD-2023
| Scenario File | Current Balance | Mean #UTXO | Current #UTXO | #Deposits | #Inputs Spent | #Withdraws | #Uneconomical outputs spent | #Change Created | #Changeless | Min Change Value | Max Change Value | Mean Change Value | Std. Dev. of Change Value | Total Fees | Mean Fees per Withdraw | Cost to Empty (10 sat/vB) | Total Cost | Min Input Size | Max Input Size | Mean Input Size | Std. Dev. of Input Size | Usage |
| --- | --- | --- | --- | ---
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1848191242)
Scenario: bustabit-tiny-hot-wallet_peak-and-tail
| Scenario File | Current Balance | Mean #UTXO | Current #UTXO | #Deposits | #Inputs Spent | #Withdraws | #Uneconomical outputs spent | #Change Created | #Changeless | Min Change Value | Max Change Value | Mean Change Value | Std. Dev. of Change Value | Total Fees | Mean Fees per Withdraw | Cost to Empty (10 sat/vB) | Total Cost | Min Input Size | Max Input Size | Mean Input Size | Std. Dev. of Input Size | Usage |
| bustabit-tiny-hot-wallet_p
...
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1848191242)
Scenario: bustabit-tiny-hot-wallet_peak-and-tail
| Scenario File | Current Balance | Mean #UTXO | Current #UTXO | #Deposits | #Inputs Spent | #Withdraws | #Uneconomical outputs spent | #Change Created | #Changeless | Min Change Value | Max Change Value | Mean Change Value | Std. Dev. of Change Value | Total Fees | Mean Fees per Withdraw | Cost to Empty (10 sat/vB) | Total Cost | Min Input Size | Max Input Size | Mean Input Size | Std. Dev. of Input Size | Usage |
| bustabit-tiny-hot-wallet_p
...
📝 ajtowns opened a pull request: "202312 vbits simplify"
(https://github.com/bitcoin/bitcoin/pull/29039)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/29039)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
📝 ajtowns converted_to_draft a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039)
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code.
(https://github.com/bitcoin/bitcoin/pull/29039)
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code.
💬 ajtowns commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-1848232374)
At the end of this sequence of patches the `VersionBitsCache` object provides a standalone interface to all the "versionbits" features needed by bitcoin core, which means that if the BIP9/speedy trial implementation is changed in future, it should be possible to do that in one place, without needing to change validation/RPC code as well.
This also simplifies and modernises some of the code, much of which has been proposed previously (eg, removing the params arguments from AbstractThresholdCon
...
(https://github.com/bitcoin/bitcoin/pull/29039#issuecomment-1848232374)
At the end of this sequence of patches the `VersionBitsCache` object provides a standalone interface to all the "versionbits" features needed by bitcoin core, which means that if the BIP9/speedy trial implementation is changed in future, it should be possible to do that in one place, without needing to change validation/RPC code as well.
This also simplifies and modernises some of the code, much of which has been proposed previously (eg, removing the params arguments from AbstractThresholdCon
...
💬 helpau commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-1848265773)
Hi, is this benchmark for HDD or SSD? Did you tried to run benchmark with huge dbcache and high height?
(https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-1848265773)
Hi, is this benchmark for HDD or SSD? Did you tried to run benchmark with huge dbcache and high height?
💬 martinus commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421360505)
@maflcko I just looked through the code a bit to find other usages of `fuzzed_data_provider` that depend on the evaluation order, and there are quite a few of them. E.g.
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/addrman.cpp#L272
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/addrman.cpp#L286-L290
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/banman.cpp#L79
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/bloom_filter.cpp#L
...
(https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1421360505)
@maflcko I just looked through the code a bit to find other usages of `fuzzed_data_provider` that depend on the evaluation order, and there are quite a few of them. E.g.
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/addrman.cpp#L272
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/addrman.cpp#L286-L290
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/banman.cpp#L79
* https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/bloom_filter.cpp#L
...
💬 maflcko commented on issue "Software wont launch":
(https://github.com/bitcoin/bitcoin/issues/29033#issuecomment-1848332041)
> Four hours have passed, software is stuck on "verifying blocks, press Q to close" initial screen.
The software is verifying the blocks that were downloaded previously. This may take a long time, depending on your system. You can look at the tail of the debug log to see the current exact status.
> I killed the process as it was only at 20% of "verifying", so I noticed it won't open for the entire day.
Killing means it'll likely have discarded the progress. This shouldn't cause any issu
...
(https://github.com/bitcoin/bitcoin/issues/29033#issuecomment-1848332041)
> Four hours have passed, software is stuck on "verifying blocks, press Q to close" initial screen.
The software is verifying the blocks that were downloaded previously. This may take a long time, depending on your system. You can look at the tail of the debug log to see the current exact status.
> I killed the process as it was only at 20% of "verifying", so I noticed it won't open for the entire day.
Killing means it'll likely have discarded the progress. This shouldn't cause any issu
...
💬 martinus commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-1848332095)
I only measured it on an SSD, and only with relatively low dbcache size so flushing is triggered more often which I think would give more accurate results. But more benchmarks are definitely appreciated :)
(https://github.com/bitcoin/bitcoin/pull/28945#issuecomment-1848332095)
I only measured it on an SSD, and only with relatively low dbcache size so flushing is triggered more often which I think would give more accurate results. But more benchmarks are definitely appreciated :)
💬 maflcko commented on issue "Software wont launch":
(https://github.com/bitcoin/bitcoin/issues/29033#issuecomment-1848332099)
You can find the debug.log in your [data dir](https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#data-directory-location).
Please be aware that the debug log might contain personally identifying information.
(https://github.com/bitcoin/bitcoin/issues/29033#issuecomment-1848332099)
You can find the debug.log in your [data dir](https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#data-directory-location).
Please be aware that the debug log might contain personally identifying information.
💬 maflcko commented on issue "test: Intermittent issue in rpc_net.py":
(https://github.com/bitcoin/bitcoin/issues/29030#issuecomment-1848332405)
Yes, I haven't looked at the code, but I presumed it was the same issue/fix.
(https://github.com/bitcoin/bitcoin/issues/29030#issuecomment-1848332405)
Yes, I haven't looked at the code, but I presumed it was the same issue/fix.
💬 maflcko commented on pull request "test: fix `addnode` functional test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/29035#issuecomment-1848360906)
lgtm. Is the code in 26.x and does it need backport?
(https://github.com/bitcoin/bitcoin/pull/29035#issuecomment-1848360906)
lgtm. Is the code in 26.x and does it need backport?
💬 theStack commented on pull request "test: fix `addnode` functional test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/29035#issuecomment-1848394286)
> lgtm. Is the code in 26.x and does it need backport?
No backport needed, the PR containing the code (#28155, commit 94e8882d820969ddc83f24f4cbe1515a886da4ea) was merged to master after 26.x branched off.
(https://github.com/bitcoin/bitcoin/pull/29035#issuecomment-1848394286)
> lgtm. Is the code in 26.x and does it need backport?
No backport needed, the PR containing the code (#28155, commit 94e8882d820969ddc83f24f4cbe1515a886da4ea) was merged to master after 26.x branched off.
📝 maflcko opened a pull request: " refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040)
This:
* Removes dead code.
* Avoids unused copies in some places.
* Adds copies in other places for safety.
(https://github.com/bitcoin/bitcoin/pull/29040)
This:
* Removes dead code.
* Avoids unused copies in some places.
* Adds copies in other places for safety.
💬 maflcko commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1848396710)
> > No objection to changing the code, but if this is worth it to change, it should be done for all places in the whole codebase, not just some places.
>
> These are all reported (by clang 18). I see `ArgsManager::GetDataDir()` has the same issue. Maybe it was not detected because of the `?:` operator. I have extended the first commit to cover that too.
Thanks, I've taken the commit and put it in https://github.com/bitcoin/bitcoin/pull/29040, because it fits into the other fs::path changes
...
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1848396710)
> > No objection to changing the code, but if this is worth it to change, it should be done for all places in the whole codebase, not just some places.
>
> These are all reported (by clang 18). I see `ArgsManager::GetDataDir()` has the same issue. Maybe it was not detected because of the `?:` operator. I have extended the first commit to cover that too.
Thanks, I've taken the commit and put it in https://github.com/bitcoin/bitcoin/pull/29040, because it fits into the other fs::path changes
...