💬 0xB10C commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3613987461)
Saw one today with https://github.com/0xB10C/bitcoin/commit/159b6fd80d8a31c01f3b62d9194cf926c007dc37. Will wait for a few more.
```
Rate-limited addr 98.97.137.54:8333 SELF-ANNOUNCEMENT!? from peer=2324 addr=98.97.137.54:46196 ua=/Satoshi:22.0.0(FutureBit-Apollo-Node)/ processed=1 rate-limited=1 age=478ms type=inbound
```
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3613987461)
Saw one today with https://github.com/0xB10C/bitcoin/commit/159b6fd80d8a31c01f3b62d9194cf926c007dc37. Will wait for a few more.
```
Rate-limited addr 98.97.137.54:8333 SELF-ANNOUNCEMENT!? from peer=2324 addr=98.97.137.54:46196 ua=/Satoshi:22.0.0(FutureBit-Apollo-Node)/ processed=1 rate-limited=1 age=478ms type=inbound
```
💬 hMsats commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3613993231)
ACK https://github.com/bitcoin/bitcoin/commit/b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3613993231)
ACK https://github.com/bitcoin/bitcoin/commit/b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
💬 darosior commented on issue "Syncing headers with feeler-peers":
(https://github.com/bitcoin/bitcoin/issues/16859#issuecomment-3614051541)
Is there any reason for keeping this open now that #19858 was merged? Good arguments for not implementing this behaviour through feeler connections were given above, and #19858 implemented this through a new type of temporary outbound connections instead.
(https://github.com/bitcoin/bitcoin/issues/16859#issuecomment-3614051541)
Is there any reason for keeping this open now that #19858 was merged? Good arguments for not implementing this behaviour through feeler connections were given above, and #19858 implemented this through a new type of temporary outbound connections instead.
🤔 stickies-v reviewed a pull request: "log: don't rate-limit "new peer" with -debug=net"
(https://github.com/bitcoin/bitcoin/pull/34008#pullrequestreview-3541816076)
Concept ACK
Another approach could be to exempt all (not just debug) logs from a debug-enabled category? I think conceptually that makes sense, and I suspect there will be more places where this is helpful so we can minimize the workarounds needed?
For example:
<details>
<summary>git diff on 9e02f78089</summary>
```diff
diff --git a/src/logging.h b/src/logging.h
index defff61d30..074096ed5f 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -377,16 +377,18 @@ inline void LogPrin
...
(https://github.com/bitcoin/bitcoin/pull/34008#pullrequestreview-3541816076)
Concept ACK
Another approach could be to exempt all (not just debug) logs from a debug-enabled category? I think conceptually that makes sense, and I suspect there will be more places where this is helpful so we can minimize the workarounds needed?
For example:
<details>
<summary>git diff on 9e02f78089</summary>
```diff
diff --git a/src/logging.h b/src/logging.h
index defff61d30..074096ed5f 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -377,16 +377,18 @@ inline void LogPrin
...
✅ sdaftuar closed an issue: "Syncing headers with feeler-peers"
(https://github.com/bitcoin/bitcoin/issues/16859)
(https://github.com/bitcoin/bitcoin/issues/16859)
🤔 darosior reviewed a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3541861752)
Concept/Approach ACK.
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3541861752)
Concept/Approach ACK.
💬 maflcko commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r2590403179)
It is an internal bug, so it is dead code and doesn't really matter much. I think internal bugs should be using `Assume(false)`. Logs could go with `LogError("%s", STR_INTERNAL_BUG("..."))`, but I wanted to keep the changes here minimal. Happy to go with `LogError("%s", STR_INTERNAL_BUG(` if you feel strongly.
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r2590403179)
It is an internal bug, so it is dead code and doesn't really matter much. I think internal bugs should be using `Assume(false)`. Logs could go with `LogError("%s", STR_INTERNAL_BUG("..."))`, but I wanted to keep the changes here minimal. Happy to go with `LogError("%s", STR_INTERNAL_BUG(` if you feel strongly.
🤔 darosior reviewed a pull request: "test: Add DERSIG unit tests to script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973#pullrequestreview-3541895187)
Makes sense to test `DERSIG` separately from `STRICTENC` since it's the only one enabled by consensus. Concept/Approach ACK.
Maintainers could we get the CI ran here?
(https://github.com/bitcoin/bitcoin/pull/33973#pullrequestreview-3541895187)
Makes sense to test `DERSIG` separately from `STRICTENC` since it's the only one enabled by consensus. Concept/Approach ACK.
Maintainers could we get the CI ran here?
💬 ryanofsky commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3614166836)
<!-- begin push-2 -->
Updated d179f713792432c82befd39a288ab16fec13bab1 -> 6ef092c0e4343fc421c323ff09d3c791fb4bc33a ([`pr/pyipc.1`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.1) -> [`pr/pyipc.2`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/pyipc.1..pr/pyipc.2))<!-- end --> splitting into two commits
re: https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611494379
> > Destroy calls were being made at
...
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3614166836)
<!-- begin push-2 -->
Updated d179f713792432c82befd39a288ab16fec13bab1 -> 6ef092c0e4343fc421c323ff09d3c791fb4bc33a ([`pr/pyipc.1`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.1) -> [`pr/pyipc.2`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/pyipc.1..pr/pyipc.2))<!-- end --> splitting into two commits
re: https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611494379
> > Destroy calls were being made at
...
🤔 stickies-v reviewed a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3541953136)
Concept ACK. Functions with return type `std::optional<E>` returning a truthy value on failure is unintuitive, this is much better.
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3541953136)
Concept ACK. Functions with return type `std::optional<E>` returning a truthy value on failure is unintuitive, this is much better.
👍 darosior approved a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541954260)
utACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541954260)
utACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe
🤔 mzumsande reviewed a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541981127)
ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe
didn't test it, but but the explanation of the bug make sense and since all the network threads have been stopped at this point anyway I can't see a downside to clearing `m_reconnections` here.
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541981127)
ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe
didn't test it, but but the explanation of the bug make sense and since all the network threads have been stopped at this point anyway I can't see a downside to clearing `m_reconnections` here.
👍 maflcko approved a pull request: "test: Remove `system_tests/run_command` runtime dependencies"
(https://github.com/bitcoin/bitcoin/pull/33929#pullrequestreview-3542024077)
lgtm
(https://github.com/bitcoin/bitcoin/pull/33929#pullrequestreview-3542024077)
lgtm
💬 maflcko commented on pull request "test: Remove `system_tests/run_command` runtime dependencies":
(https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065)
nit: This will fail with spaces in the dir:
```
$ ./a\ space/test_bitcoin -t system_tests
Running 1 test case...
unknown location(0): fatal error: in "system_tests/run_command": subprocess::CalledProcessError: execve failed: No such file or directory (2)
test/system_tests.cpp(29): last checkpoint
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
(https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065)
nit: This will fail with spaces in the dir:
```
$ ./a\ space/test_bitcoin -t system_tests
Running 1 test case...
unknown location(0): fatal error: in "system_tests/run_command": subprocess::CalledProcessError: execve failed: No such file or directory (2)
test/system_tests.cpp(29): last checkpoint
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
👍 sedited approved a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3542392743)
ACK 9d5021a05bd33c73276909eec961777867ddb412
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3542392743)
ACK 9d5021a05bd33c73276909eec961777867ddb412
👍 andrewtoth approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3542841874)
ACK 2d398050ee31db05e9222149b5e60572ac31d803
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3542841874)
ACK 2d398050ee31db05e9222149b5e60572ac31d803
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591174137)
nit
```suggestion
const auto pushed_tx_opt{m_tx_for_private_broadcast.GetTxForNode(pfrom.GetId())};
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591174137)
nit
```suggestion
const auto pushed_tx_opt{m_tx_for_private_broadcast.GetTxForNode(pfrom.GetId())};
```
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591173094)
nit
```suggestion
const auto opt_tx{m_tx_for_private_broadcast.PickTxForSend(node.GetId())};
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591173094)
nit
```suggestion
const auto opt_tx{m_tx_for_private_broadcast.PickTxForSend(node.GetId())};
```
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591176187)
nit
```suggestion
if (const auto num_broadcasted{m_tx_for_private_broadcast.Remove(ptx)}) {
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591176187)
nit
```suggestion
if (const auto num_broadcasted{m_tx_for_private_broadcast.Remove(ptx)}) {
```
⚠️ fugatecorey22 opened an issue: "Admin Dashboard"
(https://github.com/bitcoin/bitcoin/issues/34009)
npm create vite@latest myapp --template react
cd myapp
npm install
npm run dev
(https://github.com/bitcoin/bitcoin/issues/34009)
npm create vite@latest myapp --template react
cd myapp
npm install
npm run dev