💬 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__()`
(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.
(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
...
(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?
(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)`
(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.
(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`
(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
(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
...
(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?
(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
(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
...
(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?
(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?
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1607131131)
Just adding the true check in `mempool_datacarrier` seems like it would be easy?
💬 AngusP commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607133058)
Ah yes, I feel I should've remembered that 😆 it's usually considered correct-est to use the `%` formatting in log messages because that formatted string is only actually created if the message will be logged based on log-level, vs `f"` or `.format` or `" % ...` which are always created. Others may consider that premature optimisation if string-formatting due to logging has negligible perf impact 😉
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607133058)
Ah yes, I feel I should've remembered that 😆 it's usually considered correct-est to use the `%` formatting in log messages because that formatted string is only actually created if the message will be logged based on log-level, vs `f"` or `.format` or `" % ...` which are always created. Others may consider that premature optimisation if string-formatting due to logging has negligible perf impact 😉
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1607136024)
This feels overengineered. Why can't we stick to fs::perms internally and just map the string in a function?
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1607136024)
This feels overengineered. Why can't we stick to fs::perms internally and just map the string in a function?
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2121018386)
Benchmarks on my Ryzen 5950X system:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,373.94 | 421,240.11 | 0.1% | 1.10 | `LinearizeNoIters16TxWorstCase`
| 7,530.22 | 132,798.26 | 0.0% | 1.07 | `LinearizeNoIters32TxWorstCase`
| 16,585.34 | 60,294.20 | 0.1% | 1.10 | `LinearizeNoIters48TxWorstCase`
...
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2121018386)
Benchmarks on my Ryzen 5950X system:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,373.94 | 421,240.11 | 0.1% | 1.10 | `LinearizeNoIters16TxWorstCase`
| 7,530.22 | 132,798.26 | 0.0% | 1.07 | `LinearizeNoIters32TxWorstCase`
| 16,585.34 | 60,294.20 | 0.1% | 1.10 | `LinearizeNoIters48TxWorstCase`
...
💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2121032237)
Concept ACK on making the TRUC transaction vsize limit 10 kvB.
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2121032237)
Concept ACK on making the TRUC transaction vsize limit 10 kvB.
🤔 theuni reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2066845490)
Concept ACK. Nice.
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2066845490)
Concept ACK. Nice.
💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607175446)
Any reason to not make `max_size_bytes` a `CSignatureCache` constructor param? That way `InitSignatureCache` could go away.
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607175446)
Any reason to not make `max_size_bytes` a `CSignatureCache` constructor param? That way `InitSignatureCache` could go away.