🤔 jonatack reviewed a pull request: "doc: Add missing backtick in developer notes logging section"
(https://github.com/bitcoin/bitcoin/pull/29241#pullrequestreview-1818473528)
ACK, one suggestion
(https://github.com/bitcoin/bitcoin/pull/29241#pullrequestreview-1818473528)
ACK, one suggestion
💬 jonatack commented on pull request "doc: Add missing backtick in developer notes logging section":
(https://github.com/bitcoin/bitcoin/pull/29241#discussion_r1450579579)
While touching these, suggest also this
```diff
@@ -742,7 +742,7 @@ logging messages. They should be used as follows:
- `LogInfo(fmt, params...)` should only be used rarely, eg for startup
messages or for infrequent and important events such as a new block tip
being found or a new outbound connection being made. These log messages
- are unconditional so care must be taken that they can't be used by an
+ are unconditional, so care must be taken that they can't be used by an
...
(https://github.com/bitcoin/bitcoin/pull/29241#discussion_r1450579579)
While touching these, suggest also this
```diff
@@ -742,7 +742,7 @@ logging messages. They should be used as follows:
- `LogInfo(fmt, params...)` should only be used rarely, eg for startup
messages or for infrequent and important events such as a new block tip
being found or a new outbound connection being made. These log messages
- are unconditional so care must be taken that they can't be used by an
+ are unconditional, so care must be taken that they can't be used by an
...
📝 TheCharlatan converted_to_draft a pull request: "build: Remove HAVE_CONSENSUS_LIB"
(https://github.com/bitcoin/bitcoin/pull/29123)
The `script/bitcoinconsensus` module defines the public interface for the `bitcoinconsensus` library. Even though this module is only required by the tests and the `bitcoinconsensus` library, it is currently compiled into the static internal `libbitcoin_consensus` library, and therefore used by a bunch of build targets that do not require it.
Since it is always part of the internal library, the `HAVE_CONSENSUS_LIB` define used by the tests is essentially
meaningless, since the module is alwa
...
(https://github.com/bitcoin/bitcoin/pull/29123)
The `script/bitcoinconsensus` module defines the public interface for the `bitcoinconsensus` library. Even though this module is only required by the tests and the `bitcoinconsensus` library, it is currently compiled into the static internal `libbitcoin_consensus` library, and therefore used by a bunch of build targets that do not require it.
Since it is always part of the internal library, the `HAVE_CONSENSUS_LIB` define used by the tests is essentially
meaningless, since the module is alwa
...
💬 fjahr commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-1889477510)
> > `* =` in the current implementation this involves giving someone a malicious binary or getting them to recompile
>
> Not sure if this pull request is needed then.
>
> If someone is running a malicious binary or malicious code, the malicious actor will have this code removed from the binary or code.
I have been thinking about this concept for a while now and I tend to agree that this is not necessary for similar reasons as mentioned above but I am also not against this being added. H
...
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-1889477510)
> > `* =` in the current implementation this involves giving someone a malicious binary or getting them to recompile
>
> Not sure if this pull request is needed then.
>
> If someone is running a malicious binary or malicious code, the malicious actor will have this code removed from the binary or code.
I have been thinking about this concept for a while now and I tend to agree that this is not necessary for similar reasons as mentioned above but I am also not against this being added. H
...
💬 tiero commented on pull request "Implement 64 bit arithmetic op codes in the Script interpreter":
(https://github.com/bitcoin/bitcoin/pull/29221#issuecomment-1889485166)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29221#issuecomment-1889485166)
Concept ACK
💬 maflcko commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1889501254)
Maybe mark as draft while this is waiting on #29165? (29165 won't happen unless someone bumps google's oss-fuzz's clang)
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1889501254)
Maybe mark as draft while this is waiting on #29165? (29165 won't happen unless someone bumps google's oss-fuzz's clang)
💬 fjahr commented on pull request "doc: Add missing backtick in developer notes logging section":
(https://github.com/bitcoin/bitcoin/pull/29241#discussion_r1450602869)
Done, thanks!
(https://github.com/bitcoin/bitcoin/pull/29241#discussion_r1450602869)
Done, thanks!
📝 sipa converted_to_draft a pull request: "miniscript: make operator""_mst consteval"
(https://github.com/bitcoin/bitcoin/pull/28657)
It seems modern compilers don't realize that all invocations of operator""_mst can be evaluated at compile time, despite the `constexpr` keyword.
Since C++20, we can force them to evaluate at compile time using `consteval`, turning all the miniscript type constants into actual compile-time constants.
This should give a nice but not very important speedup for miniscript logic, but it's also a way to start testing C++20 features.
(https://github.com/bitcoin/bitcoin/pull/28657)
It seems modern compilers don't realize that all invocations of operator""_mst can be evaluated at compile time, despite the `constexpr` keyword.
Since C++20, we can force them to evaluate at compile time using `consteval`, turning all the miniscript type constants into actual compile-time constants.
This should give a nice but not very important speedup for miniscript logic, but it's also a way to start testing C++20 features.
💬 mzumsande commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1889521586)
> If so, it seems more natural to me to specify outbound net permissions wherever that manual connection is instructed
That would mean adding logic / additional options for it in possibly multiple spots, and having yet another place where net permissions are defined. If that becomes too messy, another option would be to leave it in `-whitelist` but just not apply it to automatic outbound connections.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1889521586)
> If so, it seems more natural to me to specify outbound net permissions wherever that manual connection is instructed
That would mean adding logic / additional options for it in possibly multiple spots, and having yet another place where net permissions are defined. If that becomes too messy, another option would be to leave it in `-whitelist` but just not apply it to automatic outbound connections.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1889525495)
I added support for generating a self-signed certificate. The authority key is printed in the log in base58 format, so it can easily be used on the SRI side.
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1889525495)
I added support for generating a self-signed certificate. The authority key is printed in the log in base58 format, so it can easily be used on the SRI side.
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1817293438)
Addressed Ishaana’s review
Added a test with a pattern of mixed weight inputs to highlight the improvements from optimizations.
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1817293438)
Addressed Ishaana’s review
Added a test with a pattern of mixed weight inputs to highlight the improvements from optimizations.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449749043)
Good catch, thanks. Fixed.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449749043)
Good catch, thanks. Fixed.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449754512)
Rephrased, especially given that there is no lookahead yet
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449754512)
Rephrased, especially given that there is no lookahead yet
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449756790)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449756790)
Fixed
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449762046)
Good point, thanks for pointing that out.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449762046)
Good point, thanks for pointing that out.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449760026)
Thanks, starting a few commits later we will SHIFT when `curr_weight` exceeds the `best_selection_weight`, so the second part of this condition can only trigger on same weight. Since we do not have the SHIFT criteria here yet, this is a bug. Fixed by explicitly checking that weight is tied.
My argument for preferring an input set with fewer funds is that the wallet’s confirmed balance is reduced less and therefore the wallet has more flexible and higher liquidity in case this transaction does
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1449760026)
Thanks, starting a few commits later we will SHIFT when `curr_weight` exceeds the `best_selection_weight`, so the second part of this condition can only trigger on same weight. Since we do not have the SHIFT criteria here yet, this is a bug. Fixed by explicitly checking that weight is tied.
My argument for preferring an input set with fewer funds is that the wallet’s confirmed balance is reduced less and therefore the wallet has more flexible and higher liquidity in case this transaction does
...
💬 m3dwards commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1889549162)
Trying to replicate this locally and at the moment I get a different failure:
```shell
assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w", "backup_w.dat")
```
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1889549162)
Trying to replicate this locally and at the moment I get a different failure:
```shell
assert_raises_rpc_error(-4, "Wallet loading failed. Error loading wallet. Wallet requires blocks to be downloaded, and software does not currently support loading wallets while blocks are being downloaded out of order when using assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height 299", n1.restorewallet, "w", "backup_w.dat")
```
💬 jonatack commented on pull request "doc: Add missing backtick in developer notes logging section":
(https://github.com/bitcoin/bitcoin/pull/29241#issuecomment-1889576187)
ACK c003562120c193c296ac754b4bb8cff02bbbe4dc
Tangential, I don't understand the macro ordering in this line:
```
The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for
```
(https://github.com/bitcoin/bitcoin/pull/29241#issuecomment-1889576187)
ACK c003562120c193c296ac754b4bb8cff02bbbe4dc
Tangential, I don't understand the macro ordering in this line:
```
The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for
```
🤔 ryanofsky reviewed a pull request: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-1818316586)
Code review f43f992b7318086859f710b920b4427e6c657fe8. I think this looks pretty close, and the approach taken in this PR to preserves backwards compatibility seems pretty clean. I would have thought prior to this PR that we might need to break backwards compatibility to convert existing named parameters to options, but this is not the case. Two things I think should be improved:
1. Lack of enforcement for m_type_per_name vector. Right now the MatchesType implementation just checks if the para
...
(https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-1818316586)
Code review f43f992b7318086859f710b920b4427e6c657fe8. I think this looks pretty close, and the approach taken in this PR to preserves backwards compatibility seems pretty clean. I would have thought prior to this PR that we might need to break backwards compatibility to convert existing named parameters to options, but this is not the case. Two things I think should be improved:
1. Lack of enforcement for m_type_per_name vector. Right now the MatchesType implementation just checks if the para
...
💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1450514731)
In commit "RPC: Support specifying different types for param aliases" (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)
I'm pretty sure you can simplify this and revert all changes to this function. All this is doing is getting the `sign` parameter type from the outer parameter declaration instead of the inner option declaration, which doesn't change anything because both types are boolean. The output of "bitcoin-cli -regtest help dump_all_command_conversions" is the same with or without this change
...
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1450514731)
In commit "RPC: Support specifying different types for param aliases" (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)
I'm pretty sure you can simplify this and revert all changes to this function. All this is doing is getting the `sign` parameter type from the outer parameter declaration instead of the inner option declaration, which doesn't change anything because both types are boolean. The output of "bitcoin-cli -regtest help dump_all_command_conversions" is the same with or without this change
...
💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1450616033)
In commit "RPC: Support specifying different types for param aliases" (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)
I don't think this check is strict enough, because it isn't actually verifying the parameter was passed with the corresponding name for the `m_type_per_name` value. It will accept a parameter with any type found in the vector, not just a parameter with the correct type for its name. This is not good because it means in the walletprocesspsbt case you could pass options=true and it w
...
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1450616033)
In commit "RPC: Support specifying different types for param aliases" (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)
I don't think this check is strict enough, because it isn't actually verifying the parameter was passed with the corresponding name for the `m_type_per_name` value. It will accept a parameter with any type found in the vector, not just a parameter with the correct type for its name. This is not good because it means in the walletprocesspsbt case you could pass options=true and it w
...