💬 martinus commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474761651)
code review & tested ACK, here are my benchmark results:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 3,822.12 | 261,634.61 | 0.3% | 22,931.00 | 8,536.02 | 2.686 | 4,512.00 | 0.1% | 0.5
...
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474761651)
code review & tested ACK, here are my benchmark results:
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 3,822.12 | 261,634.61 | 0.3% | 22,931.00 | 8,536.02 | 2.686 | 4,512.00 | 0.1% | 0.5
...
💬 mxaddict commented on pull request "doc: fix typo in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/pull/27268#issuecomment-1474785590)
> I was under the impression PRs like this were pointless time-sinks on the maintainers' time.
Makes sense
(https://github.com/bitcoin/bitcoin/pull/27268#issuecomment-1474785590)
> I was under the impression PRs like this were pointless time-sinks on the maintainers' time.
Makes sense
✅ mxaddict closed a pull request: "doc: fix typo in interface_usdt_utxocache.py"
(https://github.com/bitcoin/bitcoin/pull/27268)
(https://github.com/bitcoin/bitcoin/pull/27268)
💬 MarcoFalke commented on pull request "refactor: wallet, do not translate init options names":
(https://github.com/bitcoin/bitcoin/pull/25666#issuecomment-1474788345)
lgtm ACK e43a547a3674a31504a60ede9b4912e014a54139
(https://github.com/bitcoin/bitcoin/pull/25666#issuecomment-1474788345)
lgtm ACK e43a547a3674a31504a60ede9b4912e014a54139
💬 MarcoFalke commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474789198)
If the documentation for the field is unclear, what about changing the help text over changing the code?
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474789198)
If the documentation for the field is unclear, what about changing the help text over changing the code?
💬 MarcoFalke commented on issue "Separate RPC events from catch-all debug.log":
(https://github.com/bitcoin/bitcoin/issues/7199#issuecomment-1474790082)
Closing for now. If there is anything left to be done here, a new feature request can be submitted, or this one reopened.
(https://github.com/bitcoin/bitcoin/issues/7199#issuecomment-1474790082)
Closing for now. If there is anything left to be done here, a new feature request can be submitted, or this one reopened.
✅ MarcoFalke closed an issue: "Separate RPC events from catch-all debug.log"
(https://github.com/bitcoin/bitcoin/issues/7199)
(https://github.com/bitcoin/bitcoin/issues/7199)
💬 Almas456 commented on issue "Improve porting documentation for legacy-only wallet RPCs":
(https://github.com/bitcoin/bitcoin/issues/25363#issuecomment-1474812364)
The error message suggests that the RPC command you are trying to use, in this case 'importaddress', is not supported by the type of wallet you are using. Specifically, it seems that you are using a descriptor wallet, which does not support certain legacy-only wallet functions.
To solve the error, you can use an alternative RPC command that is compatible with descriptor wallets. In this case, you can use 'importdescriptors' instead of 'importaddress'. The 'importdescriptors' RPC command allow
...
(https://github.com/bitcoin/bitcoin/issues/25363#issuecomment-1474812364)
The error message suggests that the RPC command you are trying to use, in this case 'importaddress', is not supported by the type of wallet you are using. Specifically, it seems that you are using a descriptor wallet, which does not support certain legacy-only wallet functions.
To solve the error, you can use an alternative RPC command that is compatible with descriptor wallets. In this case, you can use 'importdescriptors' instead of 'importaddress'. The 'importdescriptors' RPC command allow
...
💬 sdaftuar commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1474874497)
utACK
I was least sure about excluding netgroups that we have MANUAL connections to. As best as I can tell, @lightlike's [reason for doing so](https://github.com/bitcoin/bitcoin/pull/19860#issuecomment-1023522150) seems to be the strongest reason:
> On the other hand, even if you do trust them, there could still be attacks on an ISP level that would apply to multiple nodes from one group, so I think it makes sense to include them in the diversification as well.
I think that makes sense
...
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1474874497)
utACK
I was least sure about excluding netgroups that we have MANUAL connections to. As best as I can tell, @lightlike's [reason for doing so](https://github.com/bitcoin/bitcoin/pull/19860#issuecomment-1023522150) seems to be the strongest reason:
> On the other hand, even if you do trust them, there could still be attacks on an ISP level that would apply to multiple nodes from one group, so I think it makes sense to include them in the diversification as well.
I think that makes sense
...
💬 LarryRuane commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474874821)
Here's a suggestion about benchmarking in general, wonder what you think. The idea is a kind of performance regression testing.
If there are a set of related benchmarks, as there are here, there may be some expected performance relationship between them. (Above I [suggested](https://github.com/bitcoin/bitcoin/pull/26957#pullrequestreview-1314607947) that Jon add a comment describing this relationship, and he [did](https://github.com/bitcoin/bitcoin/pull/26957/files#diff-b65a90c310b14b523533c7
...
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474874821)
Here's a suggestion about benchmarking in general, wonder what you think. The idea is a kind of performance regression testing.
If there are a set of related benchmarks, as there are here, there may be some expected performance relationship between them. (Above I [suggested](https://github.com/bitcoin/bitcoin/pull/26957#pullrequestreview-1314607947) that Jon add a comment describing this relationship, and he [did](https://github.com/bitcoin/bitcoin/pull/26957/files#diff-b65a90c310b14b523533c7
...
💬 jonatack commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474881729)
Thanks for the reviews, everyone.
I'm not sure it makes sense to patch around this by adding code to our CLI to parse newline escape sequences, as that would be adding unneeded complexity for 4 messages instead of fixing the problem at the source in 4 LOC for something that's probably going away anyway (`warnings`), and only fixing it for us and not for other clients.
Elaborating that last point, the 4 escape sequences were added in pulls like #15937 without warning for client software to
...
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474881729)
Thanks for the reviews, everyone.
I'm not sure it makes sense to patch around this by adding code to our CLI to parse newline escape sequences, as that would be adding unneeded complexity for 4 messages instead of fixing the problem at the source in 4 LOC for something that's probably going away anyway (`warnings`), and only fixing it for us and not for other clients.
Elaborating that last point, the 4 escape sequences were added in pulls like #15937 without warning for client software to
...
💬 jonatack commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1141095015)
As in `src/bench/peer_eviction.cpp` / `./src/bench/bench_bitcoin -filter='Eviction*.*'`, I add or keep the benchmark methods in the same alphabetical order as our bench framework prints them as a developer-friendly happiness measure. It's not required, but I think it's nice to do.
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1141095015)
As in `src/bench/peer_eviction.cpp` / `./src/bench/bench_bitcoin -filter='Eviction*.*'`, I add or keep the benchmark methods in the same alphabetical order as our bench framework prints them as a developer-friendly happiness measure. It's not required, but I think it's nice to do.
💬 hebasto commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1474952915)
Tested 46d8f32086d89de919036613b8cb437bc71a9286 on Ubuntu 22.04.
The f41493b9023acb6f3c237f6609f34f99d7e52dc1 commit introduced an error on my system:
```
$ make -C depends HOST=x86_64-apple-darwin boost FORCE_USE_SYSTEM_CLANG=1
...
Preprocessing boost...
Configuring boost...
tar: option requires an argument -- 'f'
Try 'tar --help' or 'tar --usage' for more information.
...
```
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1474952915)
Tested 46d8f32086d89de919036613b8cb437bc71a9286 on Ubuntu 22.04.
The f41493b9023acb6f3c237f6609f34f99d7e52dc1 commit introduced an error on my system:
```
$ make -C depends HOST=x86_64-apple-darwin boost FORCE_USE_SYSTEM_CLANG=1
...
Preprocessing boost...
Configuring boost...
tar: option requires an argument -- 'f'
Try 'tar --help' or 'tar --usage' for more information.
...
```
💬 jonatack commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474955390)
@LarryRuane If people prefer not to add code to do it here, perhaps reviving https://bitcoinperf.com/ to track changes in benchmarks over time, and alert when it observes a deviation threshold reached, could be a way to implement it. Maybe it could be hooked into the CI at some point.
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1474955390)
@LarryRuane If people prefer not to add code to do it here, perhaps reviving https://bitcoinperf.com/ to track changes in benchmarks over time, and alert when it observes a deviation threshold reached, could be a way to implement it. Maybe it could be hooked into the CI at some point.
💬 RandyMcMillan commented on pull request "rpc: getblock: implement with block height as input parameter.":
(https://github.com/bitcoin/bitcoin/pull/26469#issuecomment-1474956552)
Concept NACK
After further consideration:
This change sets up the condition that `getblock` has to
evaluate inputs:
```shell
getblock <hash> <int verbosity>
```
or
```shell
getblock <int> <int verbosity>
```
### This seems error prone to me!
---
Using a nested command such as:
```shell
bitcoin-cli getblock $(bitcoin-cli getblockhash 0)
bitcoin-cli getblock $(bitcoin-cli getblockhash 0) 0
bitcoin-cli getblock $(bitcoin-cli getblockhash 0) 1
bitcoin-cli getblo
...
(https://github.com/bitcoin/bitcoin/pull/26469#issuecomment-1474956552)
Concept NACK
After further consideration:
This change sets up the condition that `getblock` has to
evaluate inputs:
```shell
getblock <hash> <int verbosity>
```
or
```shell
getblock <int> <int verbosity>
```
### This seems error prone to me!
---
Using a nested command such as:
```shell
bitcoin-cli getblock $(bitcoin-cli getblockhash 0)
bitcoin-cli getblock $(bitcoin-cli getblockhash 0) 0
bitcoin-cli getblock $(bitcoin-cli getblockhash 0) 1
bitcoin-cli getblo
...
✅ russeree closed a pull request: "rpc: getblock: implement with block height as input parameter."
(https://github.com/bitcoin/bitcoin/pull/26469)
(https://github.com/bitcoin/bitcoin/pull/26469)
💬 russeree commented on pull request "rpc: getblock: implement with block height as input parameter.":
(https://github.com/bitcoin/bitcoin/pull/26469#issuecomment-1474969608)
This was meant to be closed some time ago.
Though I don't agree completely in the case of a hash vs height overload since height and hashes do not and can not intersect ever. With that said, there are already other rpc calls that use this same method to get the block with a function get hash or height.
All and all, thanks for your input.
Closed
(https://github.com/bitcoin/bitcoin/pull/26469#issuecomment-1474969608)
This was meant to be closed some time ago.
Though I don't agree completely in the case of a hash vs height overload since height and hashes do not and can not intersect ever. With that said, there are already other rpc calls that use this same method to get the block with a function get hash or height.
All and all, thanks for your input.
Closed
💬 paulkania commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#discussion_r1141172272)
not sure what this comment means.
"ever" could be a misspelt "every". If that's the case, best to change it to from 'every' to 'all the'
(https://github.com/bitcoin/bitcoin/pull/27050#discussion_r1141172272)
not sure what this comment means.
"ever" could be a misspelt "every". If that's the case, best to change it to from 'every' to 'all the'
💬 pinheadmz commented on issue "`getblocktemplate` returns a standard P2PKH with 0 sigops (testnet)":
(https://github.com/bitcoin/bitcoin/issues/24255#issuecomment-1475012160)
It's not unusual for a transaction to have 0 block-limit sigops.
The tx in your example has 9 inputs which all spend from legacy p2pkh UTXOs. In each of those spends, the `OP_CHECKSIG` is literally in the output of the previous transaction, and the sigop cost was "paid for" by the funding transactions. The inputs only contain public keys and signatures (no sigops). The single output of that tx is a P2WPKH. The signature operation required to spend that output will be "paid for" by the *spendi
...
(https://github.com/bitcoin/bitcoin/issues/24255#issuecomment-1475012160)
It's not unusual for a transaction to have 0 block-limit sigops.
The tx in your example has 9 inputs which all spend from legacy p2pkh UTXOs. In each of those spends, the `OP_CHECKSIG` is literally in the output of the previous transaction, and the sigop cost was "paid for" by the funding transactions. The inputs only contain public keys and signatures (no sigops). The single output of that tx is a P2WPKH. The signature operation required to spend that output will be "paid for" by the *spendi
...
💬 theStack commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1141224091)
In commit 225023d9e3ea5a89037ef8a4f4404a0fdd3f1cf7: This function is not used anywhere in this file and hence can be removed (or rather moved to rfc8439.cpp, see other review comment).
```suggestion
```
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1141224091)
In commit 225023d9e3ea5a89037ef8a4f4404a0fdd3f1cf7: This function is not used anywhere in this file and hence can be removed (or rather moved to rfc8439.cpp, see other review comment).
```suggestion
```