💬 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
...

💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1607107513)
To be clear, the only reason for passing `extra` here is so that it can be reserved?

If so, that's not very intuitive and seems kinda like overkill. Seems like always reserving an extra 6 bytes fine, even if they don't end up getting used?

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1607125130)
Yes, it's either 0 or 6.
I agree that we can likely always add them the end, amended my original PR: https://github.com/bitcoin/bitcoin/pull/29607/files#diff-f146300624c06d2e08aadf500952294148a1785edd6ff2e8b50f13b2c08255edR317

💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#issuecomment-2120983038)
Thanks for the review, @AngusP. Your suggestion did the trick, indeed.

<details>
<summary>Log from mining two blocks. The first exhausted the grinder search space. The second didn't.</summary>

```bash
2024-05-20 15:05:53 INFO Mined block at height 10142; next in -464h55m33s (mine)
2024-05-20 15:05:53 DEBUG Calling bitcoin-cli: ['/Users/jose.edil/2-development/bitcoin/bitcoin-core/src/bitcoin-cli', '-datadir=/Users/jose.edil/2-development/bitcoin/signet-mining-experiments/signet-fix-ha
...

💬 edilmedeiros commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2120986786)
@1ma, since you seem to have a high difficulty signet chain running, may I ask for a review in #30130?

💬 ajtowns commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1607131131)
Just adding the true check in `mempool_datacarrier` seems like it would be easy?