💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1126645252)
>Any reason to use the cli, when the normal rpc can be used directly?

When I use the normal RPC directly, I get a `CannotSendRequest` error.
> Also, I wonder if this introduces another race where the actual rescan in t.start() is scheduled after this call.

Yes, it does. I have used `assert_debug_log` with a timeout of 5 to fix this.

💬 instagibbs commented on pull request "doc: Expand scantxoutset help text to cover tr() and miniscript":
(https://github.com/bitcoin/bitcoin/pull/27155#issuecomment-1456388168)
Ok, this is the last PR like this I'll do then; going forward we should version the doc itself with updates.

Pushed updates.

👍 darosior approved a pull request: "doc: Expand scantxoutset help text to cover tr() and miniscript"
(https://github.com/bitcoin/bitcoin/pull/27155)
webACK e4ede64fe87b1619f65da5753088d93e6f481b05

📝 MarcoFalke opened a pull request: "test: Make the unlikely race in p2p_invalid_messages impossible?"
(https://github.com/bitcoin/bitcoin/pull/27212)
After `add_p2p_connection` both sides have the verack processed.
However the pong from conn in reply to the ping from the node has not
been processed and recorded in totalbytesrecv.
Flush the pong from conn by sending a ping from conn.


This should make the unlikely race impossible.

💬 MarcoFalke commented on pull request "test: Make the unlikely race in p2p_invalid_messages impossible?":
(https://github.com/bitcoin/bitcoin/pull/27212#issuecomment-1456405716)
Fixes: https://cirrus-ci.com/task/5392601050775552?logs=functional_tests#L275


````
node0 2023-03-02T14:54:19.701023Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\net_processing.cpp:3169] [ProcessMessage] [net] received: wtxidrelay (0 bytes) peer=0
node0 2023-03-02T14:54:19.701159Z [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\src\net_processing.cpp:3169] [ProcessMessage] [net] received: verack (0 bytes) peer=0
node0 2023-03-02T14:54:1
...

💬 achow101 commented on pull request "doc: Expand scantxoutset help text to cover tr() and miniscript":
(https://github.com/bitcoin/bitcoin/pull/27155#issuecomment-1456431585)
ACK e4ede64fe87b1619f65da5753088d93e6f481b05

🚀 achow101 merged a pull request: "doc: Expand scantxoutset help text to cover tr() and miniscript"
(https://github.com/bitcoin/bitcoin/pull/27155)

💬 RandyMcMillan commented on pull request "doc: Show how less noisy clang-tidy output can be achieved":
(https://github.com/bitcoin/bitcoin/pull/27205#issuecomment-1456520730)
ConceptACK

💬 john-moffett commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456593024)
I guess it's having issues with the template for the `std::optional<std::vector<uint8_t>>` instantiation, since it's not available to the linker?

Adding explicit instantiations should fix it, I think:

```diff
diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp
index 03459dc..d8b6318 100644
--- a/src/util/strencodings.cpp
+++ b/src/util/strencodings.cpp
@@ -97,6 +97,8 @@ std::optional<std::vector<Byte>> TryParseHex(std::string_view str)
}
template std::vector<std::by
...

💬 dergoegge commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1456601450)
Another crash from the `mini_miner` target:
```
YFxlvAD6+fv7+/8BAADYAAAPZWVlZWVlZV9lZWVlZWVlZWVlZWVlZWVlZWVlZWVlZWVlZWVlZWVl
ZWVlZWVlZWUgAGVlEWQX7AjobniKnjZtdcp6KPeFnFv/C89XAtDnqxwRFkNlZWVlZWVlZWVlZWVl
ZWVlZWVlZWVlZWVlZWVlZWVlZSVlZWVlZWVlY2VlZWVlZWVlZWVlZWVlZWVlZWVlZWVlZWVlZWVl
ZWVlZWVlZWVlZWVlZ2VlZWVlZWVlZWVlZWVlZWVlZWVlZWVlZWVlZWNtZWVlZWXNUm8gQwAAAGVl
ZWVlZWVlZWVlZWVlZWVlZWVlZWVclwAAtf//ZWVlAAAAAAY=
```

💬 glozow commented on pull request "test: add coverage for sigop limit policy (`-bytespersigop` setting)":
(https://github.com/bitcoin/bitcoin/pull/27171#discussion_r1126801973)
Not sure if I'm misunderstanding this comment but afaik the reason is that txsize is max(serialized size, "sigop" size), not that fees have any impact?

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126506648)
In commit "Decouple RegTestChainParams from ArgsManager" (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)

string_view would work here, don't need a full string

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126541818)
In commit "Decouple RegTestChainParams from ArgsManager" (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)

I think this could say:

```c++
options.activation_heights[*maybe_dep] = height;
```

Would also suggest renaming `maybe_dep` to `buried_deployment`.

These are just style comments, but in general this the code in the first few commits of this PR seems a little bogged down in mechanics of optional and map types instead of fluently expressing application logic. This is c++ in 2023, not
...

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126666140)
In commit "Add factory functions for Main/Test/Sig/Reg chainparams" (2ec5cdb9e4884b75623a0cc4328ac78bd5d075c9)

I think this commit makes sense, but isn't explained very well. If the goal is to replace uses of `new` with `std::make_unique` you could do that without adding a bunch of factory methods. The reason new factory methods are needed is that the existing `CreateChainParams` factory method takes an `ArgsManager` parameter, so it can't be called by libbitcoin_kernel code. By constrast, th
...

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126623251)
In commit "Decouple RegTestChainParams from ArgsManager" (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)

Unnecessary diff here, somehow brace after `for` got moved to the next line. Could try using clang-format-diff to fix.

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126765676)
re: https://github.com/bitcoin/bitcoin/pull/26177#discussion_r980405199

In commit "split non/kernel chainparamsbase" (2f2216dee7a683638e6a3e966142f50f5de7df78)

I don't think it makes sense to have `BaseParams` in the kernel at all. `BaseParams` contains things like RPC port which are at a higher level than the kernel, and are used by things like `bitcoin-cli` code and `ArgsManager` code which shouldn't depend on the kernel (see https://github.com/bitcoin/bitcoin/blob/master/doc/design/libr
...

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126832163)
In commit "Split non/kernel chainparams" (4820450688e371a99d66cfad24714fa6e041e43e)

Is calling SetGlobalBaseParams() actually necessary here? I'm guessing something breaks if isn't added but it's not clear what. It'd be good drop the call or change the generic comment above "// SETUP: Misc Globals" into a specific comment about why it's needed currently, and maybe how it can go away in the future.

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126616994)
In commit "Decouple RegTestChainParams from ArgsManager" (1cfccc38dd5d39a9e23a19b94d6ba1a7bc4c48dc)

I think it would be better to change existing `UpdateActivationParametersFromArgs(args)` line to `UpdateActivationParametersFromOptions(options)` and have that function do this stuff, instead of moving more code into this giant constructor, especially when none of the other code in the constructor touches the options struct.

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1126829031)
In commit "Split non/kernel chainparams" (4820450688e371a99d66cfad24714fa6e041e43e)

Existing `chainparams` variable is used exactly one place below so would suggest just writing `auto chainparams = CChainParams::Main();` and using it directly instead of having two similar variables.

💬 dougEfresh commented on pull request "doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2":
(https://github.com/bitcoin/bitcoin/pull/26968#issuecomment-1456693331)
@stickies-v I agree with your change, have pushed it to this PR.
Thanks