💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1606890727)
I just realized that `bitcoin-util` does not have a `help` command like `bitcoin-cli`. Looks silly now as it has only one command, but if the tool is to increase, this seems to be a fairly easy improvement someone else can work on. In case there's another PR for that, let's try to make this more close to the `bitcoin-cli` message mentioning the help command only.

💬 maflcko commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2120649012)
Maybe rebase and re-bench after 75118a608fc22a57567743000d636bc1f969f748, which replaced some copies with moves as well?

💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2120681426)
> Maybe rebase and re-bench after [75118a6](https://github.com/bitcoin/bitcoin/commit/75118a608fc22a57567743000d636bc1f969f748), which replaced some copies with moves as well?

Ah, great, thanks for pointing me to that. I had pretty much this same commit locally to address the third point in this PR's description:
> - Refactoring functions to accept UniValues by value rather than by const reference

Though I think there are still other functions that need the same treatment.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2120685605)
win64 test failure is minisketch https://github.com/bitcoin/bitcoin/pull/30072/checks?check_run_id=25181774728

💬 ryanofsky commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2120741121)
> fixing a bug introduced in b47bd959207e82555f07e028cc2246943d32d4c3: "kernel: De-globalize fReindex".

Is this true? That commit, which was part of #29817, should not have changed any previous behavior

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1606982074)
What about, instead of this, we just remove it altogether in favor of the `Commands` section that's already there? That way, it will follow the same pattern as `bitcoin-wallet`.

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1606976568)
```suggestion
"The -named option allows specifying parameters using key=value instead of positional style.\n"
```

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1606988664)
```suggestion
"It is suitable for desktop users who prefer a graphical over a command-line interface.\n\n"
```

💬 edilmedeiros commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#discussion_r1606986060)
This is not rendering nicely in the terminal:

```
Usage: bitcoin-tx [options] <hex-tx> [commands]
or: bitcoin-tx [options] -create [commands]
```

When it should be
```
Usage: bitcoin-tx [options] <hex-tx> [commands]
or: bitcoin-tx [options] -create [commands]
```

🤔 edilmedeiros reviewed a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2066570836)
Other than those comments, I compiled everything and ran the tests. Man pages seem fine too.

🤔 sr-gi reviewed a pull request: "test/BIP324: disconnection scenarios during v2 handshake"
(https://github.com/bitcoin/bitcoin/pull/29431#pullrequestreview-2066388767)
I like this approach much better, it is way easier to follow and reason about. I left some comments inline.

> because we don't send a version message to ensure that the disconnection happens in the v2 handshake phase. in your case (different from what we are testing here), we send the correct garbage terminator but because we don't send a version message - disconnection is still expected.

What is weird is that the test is expecting a specific log message, which seems to also match in this
...

💬 sr-gi commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1606879417)
In 38eb42984406dd9eabba0e3d197c7336aed495c7

This is not being used as a class attribute, so it should belong inside `__init__()`

💬 sr-gi commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1606901839)
In 6d9df282d0ca925a596787df18bf88ae48deef3a

I still think this can be simplified. `initiate_v2_handshake` is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of `magic_sent`, as long as you do the last bit manually on the test (or just create another method, like `continue_v2_handshake`).

It just feels weird to me that this initialization method is called twice (one async, once manually), it doesn't feel too intuitive.

💬 hebasto commented on pull request "wallet: Allow user to navigate options while encrypting at creation":
(https://github.com/bitcoin-core/gui/pull/722#issuecomment-2120831731)
> It looks like a "Back" button is added to the warning pop-up, but it will only take you back to enter a different password, not to disable the password selection in the create wallet dialog. Wouldn't it be better to add the "Back" button to the password dialog?
>
> Also, wouldn't it be better if the text from the two warning pop-ups was added to the top of the password dialog? Otherwise, it seems odd to first ask for a password from the user, then print two warnings about it, when the warni
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1607031740)
It could be lower. I picked 64 because I imagine somebody submitting 100s or even 1000s of transactions at the same time. Processing those serially would take a lot of time. Would you suggest another number?

💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607048554)
I'll mark those as resolved since I simplified the strings in a reviewed commit. Turns out that the script is using this style all over: `logging.debug("Mining block delta=%s start=%s mine=%s", seconds_to_hms(mine_time-bestheader["time"]), mine_time, is_mine)`

💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607049322)
Thanks, that did the job indeed.

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1607069420)
`%s`

🤔 ajtowns reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2066739761)
Concept ACK

💬 ajtowns commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607094214)
Rather than adding an error return in the constructor here, wouldn't it be better to change our cuckoo cache to clamp its size when given too large a value? ie, `InitScriptExecutionCache(135000000)` is treated as `134,217,727` instead? The value passed in is only used as an approximate target in the first place. Historical context is #25527.

<details>
<summary>(example patch)</summary>

```diff
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -360,16 +360,16 @@ public:
* struct
...