💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1140383664)
> what(): CreateChainParams: Unknown chain lol

I think you tested the wrong thing. For me it prints:

```
Error: SigNetParams: -signetchallenge cannot be multiple values.
```

Which is confusing to an end-user, because it leaks internal implementation details that are irrelevant and redundant to the user.

💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1140384430)
To test, `-signetchallenge=nonhex` silently fails/passes

💬 Xekyo commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1140387995)
You are explicitly expecting this behavior, but I’m kinda surprised that when we import a wallet that was explicitly created “blank” after the import it is now no longer blank. As far as I can tell, no keys were created, unless the import automatically does that. I’m curious why this is the expected behavior (although since you seem to explicitly expect that behavior, I don’t think there is an issue here).

💬 Xekyo commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1140372731)
Using “not” in the context of a boolean feels always a bit like a double negation. Perhaps go with “without”:

```suggestion
{RPCResult::Type::BOOL, "blank", "whether this wallet was intentionally created without keys, scripts, or descriptors"},
```

💬 ryanofsky commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474031095)
> The newline is presentation-specific and does not belong to the server output.

Again, for the record, I disagree. The RPC server does not return HTML strings or XML strings, it returns text strings, and the newline character has a very clear meaning in text strings.

Newline is a better delimiter than space if there are multiple warnings, so they are separable and coherent. Replacing newlines with spaces produces addled run-on paragraphs like "Wallet will not be encrypted. Wallet created
...

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1474048274)
Updated 593a9f36609a74f47175e681a3921f3975272766 -> 54d8644dce180197fa657e9b68d6de63cf4c8072 ([removeBlockstorageArgs_8](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_8) -> [removeBlockstorageArgs_9](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_8..removeBlockstorageArgs_9)) to improve commit messages and add a commit adding the `blockmanager_args.cpp` file to iwyu as
...

🕺 restarted server sorry

💬 achow101 commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#issuecomment-1474452399)
ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140734813)
nit:
double unneeded `()`

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140761441)
There still is a problem with this approach when the user sets a value for the single output.

Basically, the user provided value is being discarded. We don't check if it's within the inputs amount bounds nor if need to create a change output for the remaining amount (value can be lower than the inputs amount).

Current code instead of create a change output when the value is lower than the inputs amount, it sets the value equal to inputs total minus new fee. We should code a test for it too
...

👍 furszy approved a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273)
diff re-ACK 35fe41f

💬 achow101 commented on pull request "refactor: wallet, do not translate init options names":
(https://github.com/bitcoin/bitcoin/pull/25666#issuecomment-1474477293)
ACK e43a547a3674a31504a60ede9b4912e014a54139

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140811067)
Ah, I see. Actually I've decided to just rebase this PR on top of #27195.

💬 achow101 commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140812610)
Fixed.

💬 achow101 commented on pull request "bumpfee: allow send coins back to yourself":
(https://github.com/bitcoin/bitcoin/pull/27195#discussion_r1140814055)
In 7cb4dc157a711210e18f3c9b492150b6eb984b30 "bumpfee: enable send coins back to yourself"

`output_value` is not guaranteed to match what the user had actually specified for the single output.

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140814808)
Could:
```c++
auto [it, inserted] = m_records.emplace(key_data, value_data);
if (!inserted && overwrite) { // overwrite if requested
it->second = value_data;
inserted = true;
}
return inserted;
```

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140822051)
nit:
could use the previously introduced function.
```c++
GetMockableDatabase(wallet).m_pass = false;
```

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140836779)
tiny nit:
```c++
return std::make_unique<MockableDatabase>(dynamic_cast<MockableDatabase&>(database).m_records);
```

💬 furszy commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1140832310)
this include doesn't seems to be needed here.

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1140873089)
maybe I was thinking of `BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_I2P).first.IsValid());`?

not sure, I [added](https://github.com/bitcoin/bitcoin/compare/09d514583f15860f3bc7ae0c89e640c94fae3c71..17e705428ddf80c7a7f31fe5430d966cf08a37d6#diff-34d1a0e093152df355fc3a6b5b06156f7a9b936bfffb26bb221e62828e44532fR211) `BOOST_CHECK(!addrman->Select(/*new_only=*/true, NET_CJDNS).first.IsValid());` for real this time 🙃