💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1914329134)
ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464523738)
This comes unexpectedly. This is merely an unintentional side-effect of saving LOC in this test, right?

💬 naumenkogs commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1914343607)
Having these records around might be useful too, if you go back to those networks.

Do we have any practical problems with these Select calls? Too much CPU time, orz disk reads, or memory use?

Not only this would motivate this code change and review use better, but also it would help us understand under which circumstances a node operator might actually want to do this....And then maybe the right thing would be to trigger this cleaning automatically rather than thinking a user will trigger
...

💬 fanquake commented on issue "Clang 14 emits `-Wunreachable-code` warnings":
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1914349106)
> It might require some efforts if building routines involve -Werror.

Wouldn't you just pass `-Wno-unreachable-code` ?

In any case, I don't think there's anything else we can do here, other than suggest using a fixed compiler and/or stop using `-Werror`.

💬 jsarenik commented on issue "Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/issues/27494#issuecomment-1914401279)
I think that following scripts are related and may serve as inspiration while keeping backward-compatibility.

## bch.sh

For bitcoin-cli, this is bch.sh ("bitcoin cli here")

```sh
#!/bin/sh

test -d "$1" && { cd "$1"; shift; }
test -d .bitcoin && cd .bitcoin

# Handling of inside-the-wallet-dir cases
echo $PWD | grep -q '/wallets' && {
mypwd=$PWD
until test "${PWD##*/}" = "wallets"; do cd ..; done
wn=${mypwd##$PWD/}
w="-rpcwallet=${wn}"
test "$1" = "loadwallet" && a
...

👍 0xB10C approved a pull request: "test: create deterministic addrman in the functional tests"
(https://github.com/bitcoin/bitcoin/pull/29007#pullrequestreview-1848342275)
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71

Code review and played around with the new `-test` option.

💬 0xB10C commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1469388717)
nit: I think this makes the error slightly clearer
```suggestion
return InitError(Untranslated("-test=<option> can only be used when on regtest"));
```

💬 maflcko commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1469392954)
I wonder if the check could be done upfront, to not even run any test, if it is unclear whether the device will even hold enough to run the tests.

💬 stickies-v commented on pull request "test: ensure output is large enough to pay for its fees":
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1469395573)
Thanks for reporting this @maflcko, need to round the minimum value to 8 decimals to fix this. Opening a pull now.

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1469368423)
I think we should keep the word "wallet" here, since this is a client set option. If you look at the RPC call above, it differentiates between the node wide policy of `relayMinFee` and the wallet set policy `m_min_fee` by adding the world "wallet" to the RPC error.

🤔 josibake reviewed a pull request: "Wallet: Add `maxfeerate` wallet startup option"
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-1848310090)
It seems odd to me that we would ever have `maxtxfee` and `maxfeerate` set together as they seem to cover entirely different scenarios.

For `maxtxfee`, a user is expressing "I don't or care what my transaction size is but I know I never want to spend more than X in total fees."

For `maxfeerate` as user is expressing "I know that my transactions will vary a lot in size, so I don't know what the total fees will be, but I do know that I don't want to pay more than X sats/per vbyte."

I am s
...

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1469372132)
I don't think we should remove this check for setting a `max_fee` that results in fee rates less than the `relayMinFee`.

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914420467)
I added `Check()` when deserialising.

> Not familiar with the stratumv2 spec, but this seems odd to me

In Stratum v2 a pool or Template Provider (implemented in #28983) may (optionally) have an identity. This is necessary to prevent stealing hash rate (at least for the pool). In order to protect that identity in the long run, you can keep its private key on a different machine, generate a static key for the server and sign a certificate for that static key.

> not a good fit for CKey


...

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469402317)
Done

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469402366)
Added

📝 stickies-v opened a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343)
Addresses https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1468842089.

Fixes a `JSONRPCException: Invalid amount (-3)` exception by ensuring the amount sent to `sendtoaddress` is rounded to 8 decimals.

See https://cirrus-ci.com/task/5562947183837184?logs=ci#L2559

Note: since `round` can also round down, `min_amount` is not _exactly_ guaranteed, but this is not a problem for the current usage. I've added a docstring to highlight this.

💬 stickies-v commented on pull request "test: ensure output is large enough to pay for its fees":
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1469408527)
https://github.com/bitcoin/bitcoin/pull/29343/

👍 vasild approved a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1848385247)
ACK 683f988f90bd625544529b2366984bb677dd6e31

💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469415640)
No need for the extra nesting `{`.

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914474598)
I dropped support for uncompressed keys and added a comment to explain the difference with OpenSSL encoding. Also added test for invalid keys.