💬 maflcko commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445629)
Same bench result here, compiling with `clang`, both master and this pull. My command:
```
$ grep 'make_bitcoin_core=' ~/.bashrc
alias make_bitcoin_core=" ./autogen.sh && CC=clang CXX=clang++ ./configure -q --enable-c++20 --enable-experimental-util-chainstate --with-experimental-kernel-lib && make clean && make -j $(nproc)"
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445629)
Same bench result here, compiling with `clang`, both master and this pull. My command:
```
$ grep 'make_bitcoin_core=' ~/.bashrc
alias make_bitcoin_core=" ./autogen.sh && CC=clang CXX=clang++ ./configure -q --enable-c++20 --enable-experimental-util-chainstate --with-experimental-kernel-lib && make clean && make -j $(nproc)"
👍 pablomartin4btc approved a pull request: "rpc: fix getrawtransaction segfault"
(https://github.com/bitcoin/bitcoin/pull/29003#pullrequestreview-1765919970)
tACK 494a926d05df44b60b3bc1145ad2a64acf96f61b
I've managed to reproduce the issue, this PR fixes it.
(https://github.com/bitcoin/bitcoin/pull/29003#pullrequestreview-1765919970)
tACK 494a926d05df44b60b3bc1145ad2a64acf96f61b
I've managed to reproduce the issue, this PR fixes it.
💬 mzumsande commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841458226)
> A test wouldn't hurt, I'd say.
Will add one soon.
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841458226)
> A test wouldn't hurt, I'd say.
Will add one soon.
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1841458691)
You can see full-rbf replacements and their outcome on https://mempool.space/rbf#fullrbf
On December 5, 2023 1:43:28 PM EST, Kilian ***@***.***> wrote:
>Is there another source for up-to-date full-rbf pool adoption?
>
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1841458691)
You can see full-rbf replacements and their outcome on https://mempool.space/rbf#fullrbf
On December 5, 2023 1:43:28 PM EST, Kilian ***@***.***> wrote:
>Is there another source for up-to-date full-rbf pool adoption?
>
👍 ryanofsky approved a pull request: "wallet: Migrate entire address book entries to watchonly and solvables too"
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1765927605)
Code review ACK 406b71abcb72f234ddf9245a3f57e748343c774f. Just suggested change since last review
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1765927605)
Code review ACK 406b71abcb72f234ddf9245a3f57e748343c774f. Just suggested change since last review
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1416195304)
ok cool. Updated per feedback.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1416195304)
ok cool. Updated per feedback.
💬 martinus commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1841506238)
> @martinus something similar was mentioned in [#15265 (comment)](https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-457720636). However, with your approach every access would require 4 lookups.
Ha, of course; I didn't think about that at all.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1841506238)
> @martinus something similar was mentioned in [#15265 (comment)](https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-457720636). However, with your approach every access would require 4 lookups.
Ha, of course; I didn't think about that at all.
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1841506898)
Updated per feedback. Thanks @andrewtoth and @naumenkogs.
Tiny diff, only renamed `m_initial_sync_finished` to `m_is_close_to_tip` in a scripted-diff commit afc29b1.
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1841506898)
Updated per feedback. Thanks @andrewtoth and @naumenkogs.
Tiny diff, only renamed `m_initial_sync_finished` to `m_is_close_to_tip` in a scripted-diff commit afc29b1.
💬 furszy commented on pull request "rpc: encryptwallet help, mention HD seed rotation and backup requirement":
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1416203917)
It seems that we commented at the same time and I missed https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414581640. Thanks for pointing it out.
Suggestions applied. Thanks.
(https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1416203917)
It seems that we commented at the same time and I missed https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414581640. Thanks for pointing it out.
Suggestions applied. Thanks.
🤔 furszy reviewed a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1765987670)
Updated per feedback. Thanks.
Applied https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414836494 suggestions.
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1765987670)
Updated per feedback. Thanks.
Applied https://github.com/bitcoin/bitcoin/pull/28980#discussion_r1414836494 suggestions.
💬 pablomartin4btc commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841519345)
> > We might also want to change `IsBlockPruned()` so it doesn't crash when called with a `nullptr`, but I didn't do that here.
>
> I think we should avoid the use of pointers when they are assumed to not be null. In C++, references exit. However, this can be done in a follow-up.
Sorry, why not fixing the issue directly here(?):
```diff
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -585,7 +585,7 @@ const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpoin
...
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841519345)
> > We might also want to change `IsBlockPruned()` so it doesn't crash when called with a `nullptr`, but I didn't do that here.
>
> I think we should avoid the use of pointers when they are assumed to not be null. In C++, references exit. However, this can be done in a follow-up.
Sorry, why not fixing the issue directly here(?):
```diff
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -585,7 +585,7 @@ const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpoin
...
👍 pablomartin4btc approved a pull request: "rpc: encryptwallet help, mention HD seed rotation and backup requirement"
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1766001270)
reACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
(https://github.com/bitcoin/bitcoin/pull/28980#pullrequestreview-1766001270)
reACK e6b9a6dba79144cbe77c59515541f9019d1db4b5
💬 mzumsande commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841540970)
> Sorry, why not fixing the issue directly here(?):
I initially decided against this because
1) calling `IsBlockPruned()` doesn't make any sense in the first place when there is no block.
2) There were lots of refactors in the blockstorage module over the last year and I wasn't sure how cleanly this would backport.
I think that it's not one or the other, both changes make sense. However, to avoid the question if `IsBlockPruned()` should return true or false for a `nullptr`, it would prob
...
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841540970)
> Sorry, why not fixing the issue directly here(?):
I initially decided against this because
1) calling `IsBlockPruned()` doesn't make any sense in the first place when there is no block.
2) There were lots of refactors in the blockstorage module over the last year and I wasn't sure how cleanly this would backport.
I think that it's not one or the other, both changes make sense. However, to avoid the question if `IsBlockPruned()` should return true or false for a `nullptr`, it would prob
...
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416220032)
yeah, it could be `NO_KEY_TIME` or could also return an optional.
The issue that I see with the `NO_KEY_TIME` constant is where to place it. Because, if we place it at the wallet.h level, we would add a circular dependency wallet -> spkm -> wallet.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416220032)
yeah, it could be `NO_KEY_TIME` or could also return an optional.
The issue that I see with the `NO_KEY_TIME` constant is where to place it. Because, if we place it at the wallet.h level, we would add a circular dependency wallet -> spkm -> wallet.
💬 furszy commented on pull request "wallet: birth time update during tx scanning":
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416225122)
> Seems like `NO_KEY_TIME` ought to be `0` so it triggers a full scan
That was the bug I solved 0a1107d0.
By default, the legacy spkm is empty. It has no keys nor scripts, thus it should not trigger a full scan.
Once the first key is generated or the first script is imported,
the legacy spkm will update the `nTimeFirstKey` time automatically and start scanning blocks.
(https://github.com/bitcoin/bitcoin/pull/28920#discussion_r1416225122)
> Seems like `NO_KEY_TIME` ought to be `0` so it triggers a full scan
That was the bug I solved 0a1107d0.
By default, the legacy spkm is empty. It has no keys nor scripts, thus it should not trigger a full scan.
Once the first key is generated or the first script is imported,
the legacy spkm will update the `nTimeFirstKey` time automatically and start scanning blocks.
🤔 murchandamus requested changes to a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1765984823)
I realized that we need to add the input fee to each UTXO rather than deducting it from the target for the two input set options to have the same waste score.
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1765984823)
I realized that we need to add the input fee to each UTXO rather than deducting it from the target for the two input set options to have the same waste score.
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416203316)
> Looks good! I am uncomfortable with adding the log message in [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) for now and r
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416203316)
> Looks good! I am uncomfortable with adding the log message in [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) as it has nothing to do with the bug fix and I think it could confuse (or worse, alarm) users to see "Waste" in their logs in the context of coin selection when waste is a not well-documented, implementation-specific term. Can we drop [a4c3294](https://github.com/bitcoin/bitcoin/commit/a4c3294b11b78ea0ec6bb19e71aa84a3571df1f5) for now and r
...
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416221619)
… drop the deducted fee:
```suggestion
add_coin(9 * CENT, /*nInput=*/0, expected_result);
add_coin(1 * CENT, /*nInput=*/0, expected_result);
const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb);
```
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416221619)
… drop the deducted fee:
```suggestion
add_coin(9 * CENT, /*nInput=*/0, expected_result);
add_coin(1 * CENT, /*nInput=*/0, expected_result);
const auto result12 = SelectCoins(*wallet, available_coins, /*pre_set_inputs=*/{}, 10 * CENT, coin_control, coin_selection_params_bnb);
```
💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416220620)
I just realized that this breaks the test. Deducting the fee of a specific count of inputs from the target breaks the equivalence of the input sets. The two inputs 9+1 now have exactly the target amount, whereas the single input of 10 will lead to an excess of one input fee. This would mean that whatever the feerate and long-term feerate, the solution with 9 and 1 would be preferred.
Instead of deducting the fee of two inputs from the target, this test should be fixed by adding one input fee
...
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1416220620)
I just realized that this breaks the test. Deducting the fee of a specific count of inputs from the target breaks the equivalence of the input sets. The two inputs 9+1 now have exactly the target amount, whereas the single input of 10 will lead to an excess of one input fee. This would mean that whatever the feerate and long-term feerate, the solution with 9 and 1 would be preferred.
Instead of deducting the fee of two inputs from the target, this test should be fixed by adding one input fee
...
🤔 ryanofsky reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1765995922)
Concept ACK, and started reviewing. But I don't think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it's actually needed or just desired for some other reason.
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1765995922)
Concept ACK, and started reviewing. But I don't think I understand the reason for adding a new flag to indicate the presence of an ACTIVEHDKEY record, when it seems like code would be simpler and more reliable just handling absence of the record as necessary. It would be helpful if PR description mentioned the reason for adding a new flag and whether it's actually needed or just desired for some other reason.