💬 sr-gi commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1548041767)
e6d7bcf7689e38dcda45a1cdbff98fd610864e47 nit: _appear_

💬 kosuodhmwa commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2034361865)
**The source code from the master branch i've compiled is:**
**_root@debian-12-btc-node:~/bitcoin# git rev-parse HEAD_** gives me the following output:
0fa9f17332a6d9b2eb6e3d9f9102bfd2d9c6f516

💬 kosuodhmwa commented on issue "Tons of Socks5() connect to x.x.x.x:8333 failed: connection refused-messages when i use TOR - why?":
(https://github.com/bitcoin/bitcoin/issues/29759#issuecomment-2034363274)

`0fa9f17332a6d9b2eb6e3d9f9102bfd2d9c6f516`

📝 vasild opened a pull request: "Logging cleanup"
(https://github.com/bitcoin/bitcoin/pull/29798)
* Move special cases from `LOG_CATEGORIES_BY_STR` to `GetLogCategory()` (suggested [here](https://github.com/bitcoin/bitcoin/pull/29419#discussion_r1547990373)).

* Remove `"none"` and `"0"` from RPC `logging` help because that help text was wrong. `"none"` resulted in an error and `"0"` was ignored itself (contrary to what the help text suggested).

* Remove unused `LOG_CATEGORIES_BY_STR[""]` (suggested [here](https://github.com/bitcoin/bitcoin/pull/29419#discussion_r1548018694)).

This i
...

💬 naiyoma commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1549560026)
would it be beneficial to add an assertion for a small scriptSig with an empty witness stack?

```
# small scriptSig, empty witness stack
self.assertEqual(calculate_input_weight(scriptSig_small, []),
(SKELETON_BYTES + SMALL_LEN_BYTES + 252) * WITNESS_SCALE_FACTOR)

```

💬 vasild commented on pull request "log: deduplicate category names and improve logging.cpp":
(https://github.com/bitcoin/bitcoin/pull/29419#issuecomment-2034378458)
@ryanofsky, thanks for the review! Addressed your pending suggestions in https://github.com/bitcoin/bitcoin/pull/29798.

💬 Sjors commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2034388617)
@maflcko yes, e.g. it built boost and zmq just fine.

💬 glozow commented on pull request "refactor: Use typesafe Wtxid in compact block encodings":
(https://github.com/bitcoin/bitcoin/pull/29752#discussion_r1549592075)
A relatively simple test could be to a `extra_txn.resize(1)` within src/test/blockencodings_tests.cpp (currently it is empty)?

This bug description looks correct to me.

🤔 glozow reviewed a pull request: "refactor: Use typesafe Wtxid in compact block encodings"
(https://github.com/bitcoin/bitcoin/pull/29752#pullrequestreview-1976573938)
ACK e178a52647fb6a8844191961fbf74266e22d5df8

The second commit seems sensible as wtxid is cached instead of being calculated on the fly since fac1223a568fa1ad6dd602350598eed278d115e8 (nit maybe mention in the commit message), though I don't think having the pair is that bad either.

💬 glozow commented on pull request "refactor: Use typesafe Wtxid in compact block encodings":
(https://github.com/bitcoin/bitcoin/pull/29752#discussion_r1549601987)
nit: I generally prefer to add braces or put these on one line (also [see](https://stackoverflow.com/questions/12193170/whats-the-purpose-of-using-braces-i-e-for-a-single-line-if-or-loop))

💬 fanquake commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2034408348)
> The regression is in the pre-built 27.0rc1 binary for Windows

Ok. It would be good if someone else on Windows can confirm this. Can you also let us know if you're using any particular config options etc.

💬 maflcko commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1549650210)
not sure. Just because it currently isn't called with that value, I fail to see what prevents that in the future.

💬 furszy commented on pull request "Fix create unsigned transaction fee bump":
(https://github.com/bitcoin-core/gui/pull/812#discussion_r1549656233)
The change was introduced because the OS system notification wasn't popping up on macOS. Either way is ok for me.

📝 willcl-ark opened a pull request: "Don't permit port in proxy IP option"
(https://github.com/bitcoin-core/gui/pull/813)
Fixes: #809

Previously it was possible through the GUI to enter an IP address:port into the "Proxy IP" configuration box. After the node was restarted the errant setting would prevent the node starting back up until manually removed from settings.json.

Prevent this with a simple check for ":" in the string. This is acceptable here in the GUI setting because we already fail on a hostname such as "http://x.x.x.x", so it won't cause false positives.

<!--
*** Please remove the following he
...

💬 jonatack commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2034505025)
See https://github.com/bitcoin/bitcoin/pull/27231.

💬 furszy commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#discussion_r1549695878)
`CreatedTransactionResult` is an existing struct. The suggestion wouldn't compile.
We could rename the struct if that is generating confusion.

🤔 tdb3 reviewed a pull request: "test: Bump timeouts in feature_index_prune and wallet_importdescriptors"
(https://github.com/bitcoin/bitcoin/pull/29791#pullrequestreview-1976750902)
Nice work @cbergqvist. The code changes lgtm.

It may be worth investigating why these tests are taking longer to run, and if that is expected or a potential performance regression that went unnoticed until now. I'm assuming that the tests were passing at some point with the existing timeout values (and that those values were chosen for a reason).

I don't have all the history on this, so I could be missing something. Perhaps it's dependent on how the tests are run (e.g. `--jobs` chosen), and if
...

🤔 maflcko reviewed a pull request: "Simplify network-adjusted time warning logic"
(https://github.com/bitcoin/bitcoin/pull/29623#pullrequestreview-1976762473)
lgtm 2ef71c73582be554e565ada3f8a6ca77c2cd79f1 🛴

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm 2ef71c73582be554e565ad
...

💬 maflcko commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549717587)
```suggestion
"this may lead to consensus failure. After you've confirmed your computer's clock, this message "
```

nit: The clock may or may not already be correct, so there may be nothing to correct, only to confirm.

💬 maflcko commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1549711736)
I don't think the use of "GUI" is correct here, is it?

The GUI only calls `getWarnings()`, aka `GetWarnings(true)`, aka `warnings_concise`, which is never set in this pull request.

Currently it is only set for RPC and the "No-UI" interface.

💬 maflcko commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#issuecomment-2034583200)
Reminder for myself: https://github.com/bitcoin/bitcoin/pull/29623#discussion_r1535417573