💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420936262)
Thanks, great suggestion. Adopted, but with turned around comparison, it needs to be less-than (I checked with a test).
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420936262)
Thanks, great suggestion. Adopted, but with turned around comparison, it needs to be less-than (I checked with a test).
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420968134)
I just need this to be as big as the largest or bigger than any of the actual groups’ input weights. Changed to `std::numeric_limits<int>::max()`
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420968134)
I just need this to be as big as the largest or bigger than any of the actual groups’ input weights. Changed to `std::numeric_limits<int>::max()`
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420959370)
Inlined the `add_utxo_at_index` function
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420959370)
Inlined the `add_utxo_at_index` function
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420947210)
Added multiplication operator in https://github.com/bitcoin/bitcoin/pull/29037, and amended here
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420947210)
Added multiplication operator in https://github.com/bitcoin/bitcoin/pull/29037, and amended here
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-1847787491)
Rebased after #28894 merge, next in the line is #28987.
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-1847787491)
Rebased after #28894 merge, next in the line is #28987.
💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420997086)
Renamed to outpoint here and elsewhere.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420997086)
Renamed to outpoint here and elsewhere.
💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420997175)
Done
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420997175)
Done
💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998401)
`GetTransactionInputWeight` returns an `int64_t`, so I've unified these to use `int64_t` as well.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998401)
`GetTransactionInputWeight` returns an `int64_t`, so I've unified these to use `int64_t` as well.
💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998560)
Good point. changed to use optionals.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998560)
Good point. changed to use optionals.
💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998652)
Done
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998652)
Done
💬 hebasto commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847830742)
> Looks like configure doesn't detect g++-10 as a C++20 compiler...
> I presume this will be fixed by cmake...
It [does](https://github.com/hebasto/bitcoin/pull/60) :)
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847830742)
> Looks like configure doesn't detect g++-10 as a C++20 compiler...
> I presume this will be fixed by cmake...
It [does](https://github.com/hebasto/bitcoin/pull/60) :)
💬 fanquake commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847839860)
Isn't it just applying the exact same workaround (internally) that we are using? There's no other way to know that GCC supports C++20, because of the bug we discussed above.
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847839860)
Isn't it just applying the exact same workaround (internally) that we are using? There's no other way to know that GCC supports C++20, because of the bug we discussed above.
💬 hebasto commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847850612)
> Isn't it just applying the exact same workaround (internally) that we are using? There's no other way to know that GCC supports C++20, because of the bug we discussed above.
I didn't look into CMake code for that. I only tested it, including the minimum supported CMake 3.16.
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847850612)
> Isn't it just applying the exact same workaround (internally) that we are using? There's no other way to know that GCC supports C++20, because of the bug we discussed above.
I didn't look into CMake code for that. I only tested it, including the minimum supported CMake 3.16.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1847884788)
After offline discussion with @sdaftuar , I've worked on a set of prospective changes and pushed them to this branch as follow-on commits.
Key change is that we will now only allow package RBF when the conflicted transactions are all in "clusters" of up to size 2. This allows us to calculate mining scores for each conflicted transaction, which means that post-cluster mempool, if we did this right, IIUC, the behavior should match with more general package RBF.
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1847884788)
After offline discussion with @sdaftuar , I've worked on a set of prospective changes and pushed them to this branch as follow-on commits.
Key change is that we will now only allow package RBF when the conflicted transactions are all in "clusters" of up to size 2. This allows us to calculate mining scores for each conflicted transaction, which means that post-cluster mempool, if we did this right, IIUC, the behavior should match with more general package RBF.
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1847943885)
Benchmark crash should be fixed.
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1847943885)
Benchmark crash should be fixed.
💬 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