💬 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;
}
```
💬 monlovesmango commented on issue "30.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/33369#issuecomment-3315487388)
Testing guide looks great! Some feedback below.
### 6.1 TRUC transactions
- Perhaps make the pull request number (listed at the end of the intro paragraph) linkable like other sections?
- This guide isn't the right place for this feedback item but was hoping for guidance on whether it is expected, and if not where to report this. If I create a tx3 that is a child of tx2 (all as v3 txs) the `send` command fails silently. It failing is the correct behavior because the package size shouldn't be gr
...
(https://github.com/bitcoin/bitcoin/issues/33369#issuecomment-3315487388)
Testing guide looks great! Some feedback below.
### 6.1 TRUC transactions
- Perhaps make the pull request number (listed at the end of the intro paragraph) linkable like other sections?
- This guide isn't the right place for this feedback item but was hoping for guidance on whether it is expected, and if not where to report this. If I create a tx3 that is a child of tx2 (all as v3 txs) the `send` command fails silently. It failing is the correct behavior because the package size shouldn't be gr
...
👋 l0rinc's pull request is ready for review: "log: don't rate limit "rolling forward" messages"
(https://github.com/bitcoin/bitcoin/pull/33443)
(https://github.com/bitcoin/bitcoin/pull/33443)
💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3315535475)
> On a conceptual level, this seems like a strong fingerprinting vector (tor/clearnet response would be identical if requests happen roughly at the same time and the same template is used). I'm not sure if we have given up on that, considering that there seems to be no lack of other existing fingerprinting methods.
This might be a little better than it seems at first glance, because the top 2MvB of txs is likely to be fairly common across nodes running similar mempool acceptance policies, and
...
(https://github.com/bitcoin/bitcoin/pull/33191#issuecomment-3315535475)
> On a conceptual level, this seems like a strong fingerprinting vector (tor/clearnet response would be identical if requests happen roughly at the same time and the same template is used). I'm not sure if we have given up on that, considering that there seems to be no lack of other existing fingerprinting methods.
This might be a little better than it seems at first glance, because the top 2MvB of txs is likely to be fairly common across nodes running similar mempool acceptance policies, and
...
💬 ajtowns commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2365971591)
Not really. When only generating the templates and not requesting them it simplifies the locking slightly, and it takes a cs_main lock to generate the block, so most other processing can't continue anyway.
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2365971591)
Not really. When only generating the templates and not requesting them it simplifies the locking slightly, and it takes a cs_main lock to generate the block, so most other processing can't continue anyway.
💬 ajtowns commented on pull request "log: don't rate limit "rolling forward" messages":
(https://github.com/bitcoin/bitcoin/pull/33443#issuecomment-3315626106)
Should call this `LogEssential()` or similar, rather than calling internal logging functions. cf https://github.com/ajtowns/bitcoin/commits/202509-logessential/
Is this information actually very useful though, or is it only relevant for debugging? There's already the `Replaying blocks...` notification which should show progress. Would something like this make more sense?
```c++
// Roll forward from the forking point to the new tip.
int nForkHeight = pindexFork ? pindexFork->nHe
...
(https://github.com/bitcoin/bitcoin/pull/33443#issuecomment-3315626106)
Should call this `LogEssential()` or similar, rather than calling internal logging functions. cf https://github.com/ajtowns/bitcoin/commits/202509-logessential/
Is this information actually very useful though, or is it only relevant for debugging? There's already the `Replaying blocks...` notification which should show progress. Would something like this make more sense?
```c++
// Roll forward from the forking point to the new tip.
int nForkHeight = pindexFork ? pindexFork->nHe
...
⚠️ Horlabrainmoore opened an issue: "python3 -m venv venv && source venv/bin/activate pip install pandas matplotlib PyPDF2 python-dateutil # (optional) pip install pdfminer.six if PyPDF2 extraction fails"
(https://github.com/bitcoin/bitcoin/issues/33447)
[# File: integrated_wallet_and_whitepaper_report.py
# Usage: adjust CSV_PATH and PDF_PATH then run: python integrated_wallet_and_whitepaper_report.py
from pathlib import Path
import re
import sys
import json
from collections import Counter, defaultdict
import pandas as pd
import matplotlib.pyplot as plt
from dateutil import parser as dateparser
# PDF extraction
try:
from PyPDF2 import PdfReader
except Exception:
PdfReader = None
# -------- CONFIG --------
CSV_PATH = Path("/mnt/data/W
...
(https://github.com/bitcoin/bitcoin/issues/33447)
[# File: integrated_wallet_and_whitepaper_report.py
# Usage: adjust CSV_PATH and PDF_PATH then run: python integrated_wallet_and_whitepaper_report.py
from pathlib import Path
import re
import sys
import json
from collections import Counter, defaultdict
import pandas as pd
import matplotlib.pyplot as plt
from dateutil import parser as dateparser
# PDF extraction
try:
from PyPDF2 import PdfReader
except Exception:
PdfReader = None
# -------- CONFIG --------
CSV_PATH = Path("/mnt/data/W
...
✅ fanquake closed an issue: "python3 -m venv venv && source venv/bin/activate pip install pandas matplotlib PyPDF2 python-dateutil # (optional) pip install pdfminer.six if PyPDF2 extraction fails"
(https://github.com/bitcoin/bitcoin/issues/33447)
(https://github.com/bitcoin/bitcoin/issues/33447)