🤔 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
...
(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__()`
(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.