💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365757547)
Although using class has no performance issues, but I think the stateless point is valid, and using namespace here is a better choice to group related functions and for readability. This will also remove indirection function calls.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365757547)
Although using class has no performance issues, but I think the stateless point is valid, and using namespace here is a better choice to group related functions and for readability. This will also remove indirection function calls.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365757618)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365757618)
Thanks, fixed.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365757992)
Thanks for suggesting. Fixed in the latest commit.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365757992)
Thanks for suggesting. Fixed in the latest commit.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365758773)
Agreed. Now in the latest commit, I removed this and shifted it above the function with more descriptive info and an example command.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2365758773)
Agreed. Now in the latest commit, I removed this and shifted it above the function with more descriptive info and an example command.
💬 Sjors commented on pull request "rpc: fix getblock(header) returns target for tip":
(https://github.com/bitcoin/bitcoin/pull/33446#issuecomment-3315194787)
I split 88fb025c83c7aa84766a5385e6a797102dd94513 in two commits: one that adds the mock mainnet block and updates the docs, and one that's focussed on fixing the actual bug and adding test coverage for it (using this new block).
(https://github.com/bitcoin/bitcoin/pull/33446#issuecomment-3315194787)
I split 88fb025c83c7aa84766a5385e6a797102dd94513 in two commits: one that adds the mock mainnet block and updates the docs, and one that's focussed on fixing the actual bug and adding test coverage for it (using this new block).
💬 BitByBitByBitByBit commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3315273628)
> > Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
>
> Friendly ping to coordinators for addressing issues:
>
> * @sr-gi -- Catalan (ca)
> * @ostruvek -- Czech (cs)
> * @pryds -- Danish (da)
> * @laanwj @sipa -- Dutch (nl)
> * @Emzy -- German (de)
> * @cryptomeow -- Greek (el)
> * @jesterhodl -- Polish (pl)
>
> UPD. French (fr) and Spanish (es) coordinators have been notified via Transifex messages.
>
> UPD
...
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3315273628)
> > Sure, see https://github.com/maflcko/b-c-gui-translations-review/tree/99bf41c4e5ab74eee8c248b240e8e940e47e09ec/reviews
>
> Friendly ping to coordinators for addressing issues:
>
> * @sr-gi -- Catalan (ca)
> * @ostruvek -- Czech (cs)
> * @pryds -- Danish (da)
> * @laanwj @sipa -- Dutch (nl)
> * @Emzy -- German (de)
> * @cryptomeow -- Greek (el)
> * @jesterhodl -- Polish (pl)
>
> UPD. French (fr) and Spanish (es) coordinators have been notified via Transifex messages.
>
> UPD
...
🤔 hodlinator reviewed a pull request: "qa: Improvements to debug_assert_log + busy_wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3244741286)
Thanks for the feedback!
> Thanks for reducing useless work - did you measure any speedup?
Rebased new push on top of #33336 which has a lot of `assert_debug_log()` (010cf81407c0df8de766fd2a116463d180f25f33), the below represents somewhat average runs:
```
₿ git co 2025/09/assert_debug_log_rebased~3
₿ hyperfine -r 30 -N ./build/test/functional/feature_assumevalid.py
Benchmark 1: ./build/test/functional/feature_assumevalid.py
Time (mean ± σ): 6.058 s ± 0.049 s [User: 4.263 s
...
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3244741286)
Thanks for the feedback!
> Thanks for reducing useless work - did you measure any speedup?
Rebased new push on top of #33336 which has a lot of `assert_debug_log()` (010cf81407c0df8de766fd2a116463d180f25f33), the below represents somewhat average runs:
```
₿ git co 2025/09/assert_debug_log_rebased~3
₿ hyperfine -r 30 -N ./build/test/functional/feature_assumevalid.py
Benchmark 1: ./build/test/functional/feature_assumevalid.py
Time (mean ± σ): 6.058 s ± 0.049 s [User: 4.263 s
...
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2362983549)
1) Agree `join_log` is more accurate, changed.
2) It felt like the setup code was all before the `yield`.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2362983549)
1) Agree `join_log` is more accurate, changed.
2) It felt like the setup code was all before the `yield`.
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364601882)
Done.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364601882)
Done.
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2362813877)
I prefer `!s` since it is closer to the original `str()`. `!r` seems more appropriate when one wants to copy the result into code, mirroring `repr()`, feverishly escaping backslashes etc:
```python
>>> repr(r'\w')
"'\\\\w'"
>>> str(r'\w')
'\\w'
```
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2362813877)
I prefer `!s` since it is closer to the original `str()`. `!r` seems more appropriate when one wants to copy the result into code, mirroring `repr()`, feverishly escaping backslashes etc:
```python
>>> repr(r'\w')
"'\\\\w'"
>>> str(r'\w')
'\\w'
```
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364558825)
We only report one `found`-entry per found expected message if it exists in the log, doesn't matter if it exists twice in the log (the indexes are not into the log), so I think it should be alright.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364558825)
We only report one `found`-entry per found expected message if it exists in the log, doesn't matter if it exists twice in the log (the indexes are not into the log), so I think it should be alright.
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364544756)
Agreed that it's nicer not modify inputs, done!
I had the list comprehension of
```python
remaining = [e for e in remaining if e not in log]
```
Then I realized it was wasteful to keep searching if we already failed to find one of the expected messages, so I reverted the list comprehension and added a `break`.
Found another way to probably get rid of `found` (not well tested). But it comes at the expense of copying the remaining enumerated `list` for each outer loop iteration, so holdi
...
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364544756)
Agreed that it's nicer not modify inputs, done!
I had the list comprehension of
```python
remaining = [e for e in remaining if e not in log]
```
Then I realized it was wasteful to keep searching if we already failed to find one of the expected messages, so I reverted the list comprehension and added a `break`.
Found another way to probably get rid of `found` (not well tested). But it comes at the expense of copying the remaining enumerated `list` for each outer loop iteration, so holdi
...
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364603250)
Moved.
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364603250)
Moved.
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364576723)
At first I thought there was some special property with `re.search(..., re.MULTILINE)`, but digging deeper I think not. Looking at the PR which added it, I found https://github.com/bitcoin/bitcoin/pull/14024#discussion_r212662890 which indicates that the original user of the function was passing in a search string with symbols that could be interpreted as a regex but shouldn't be. #13006 which spawned the issue mentioned regexps, so that's probably why it's in this unfortunate nerfed state.
I
...
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2364576723)
At first I thought there was some special property with `re.search(..., re.MULTILINE)`, but digging deeper I think not. Looking at the PR which added it, I found https://github.com/bitcoin/bitcoin/pull/14024#discussion_r212662890 which indicates that the original user of the function was passing in a search string with symbols that could be interpreted as a regex but shouldn't be. #13006 which spawned the issue mentioned regexps, so that's probably why it's in this unfortunate nerfed state.
I
...
👍 hodlinator approved a pull request: "log: always print initial script verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3249291256)
re-ACK 010cf81407c0df8de766fd2a116463d180f25f33
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3249291256)
re-ACK 010cf81407c0df8de766fd2a116463d180f25f33
💬 hodlinator commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2365841816)
Warp speed engaged, thank you!
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2365841816)
Warp speed engaged, thank you!
💬 enirox001 commented on pull request "rpc: Fix dumptxoutset rollback with competing forks":
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3315294250)
@fjahr @mzumsande Would appreciate your feedback on this implementation when you get a chance. Thanks!
(https://github.com/bitcoin/bitcoin/pull/33444#issuecomment-3315294250)
@fjahr @mzumsande Would appreciate your feedback on this implementation when you get a chance. Thanks!
🤔 pablomartin4btc reviewed a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3249356226)
tested 862faf3fa7a42238017f8d673b0c656531c688dc
This version doesn't work properly and produces the same issue it's trying to fix as every call to `MakeWalletDatabase()` within conditionals in `VerifyWallets()` (_`src/wallet/load.cpp`_), ends up also calling the SQLiteDatabase::Cleanup() in the destroyer which does `--g_sqlite_count`.
<details>
<summary><i>(tiny nit: only if you have to re-touch, there's a typo in commit message body)</i></summary>
<img width="742" height="253" alt="
...
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3249356226)
tested 862faf3fa7a42238017f8d673b0c656531c688dc
This version doesn't work properly and produces the same issue it's trying to fix as every call to `MakeWalletDatabase()` within conditionals in `VerifyWallets()` (_`src/wallet/load.cpp`_), ends up also calling the SQLiteDatabase::Cleanup() in the destroyer which does `--g_sqlite_count`.
<details>
<summary><i>(tiny nit: only if you have to re-touch, there's a typo in commit message body)</i></summary>
<img width="742" height="253" alt="
...
💬 l0rinc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3315461001)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3315461001)
Concept ACK
💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2365920111)
what about (and no longer need to check `g_sqlite_count`)...
```suggestion
static bool sqlite_version_logged = false;
if (!sqlite_version_logged) {
LogInfo("Using SQLite Version %s", SQLiteDatabaseVersion());
sqlite_version_logged = true;
}
```
(https://github.com/bitcoin/bitcoin/pull/33299#discussion_r2365920111)
what about (and no longer need to check `g_sqlite_count`)...
```suggestion
static bool sqlite_version_logged = false;
if (!sqlite_version_logged) {
LogInfo("Using SQLite Version %s", SQLiteDatabaseVersion());
sqlite_version_logged = true;
}
```