💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1682735790)
Updated per `git range-diff 6ce5e8f ce8faad 05143b2` to take review feedback by @ajtowns (thanks!) and rebase for CI.
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1682735790)
Updated per `git range-diff 6ce5e8f ce8faad 05143b2` to take review feedback by @ajtowns (thanks!) and rebase for CI.
📝 mzumsande opened a pull request: "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation"
(https://github.com/bitcoin/bitcoin/pull/28287)
This adds a `sendmsgtopeer` rpc (for testing only) that allows a node to send a message (provided in hex) to a peer.
While we would usually use a `p2p` object instead of a node for this in the test framework, that isn't possible in situations where this message needs to trigger an actual interaction of multiple nodes.
Use this rpc to add test coverage for the bug fixed in #27981 (that just got merged):
The test lets two nodes (almost) simultaneously send a single large (4MB) p2p message to
...
(https://github.com/bitcoin/bitcoin/pull/28287)
This adds a `sendmsgtopeer` rpc (for testing only) that allows a node to send a message (provided in hex) to a peer.
While we would usually use a `p2p` object instead of a node for this in the test framework, that isn't possible in situations where this message needs to trigger an actual interaction of multiple nodes.
Use this rpc to add test coverage for the bug fixed in #27981 (that just got merged):
The test lets two nodes (almost) simultaneously send a single large (4MB) p2p message to
...
💬 luke-jr commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#discussion_r1276773348)
Extraneous `\"` shouldn't be there, this is an array
(https://github.com/bitcoin/bitcoin/pull/27351#discussion_r1276773348)
Extraneous `\"` shouldn't be there, this is an array
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1297598582)
I believe that's true, though only due to optimistic send itself (which guarantees that as soon as something is added to `vSendMsg`, it's immediately at least attempted to be moved to the transport). Without it, if there were two consecutive `PushMessage` without intervening `SocketSendData`, `vSendMsg` would already have something in it on the second one.
Because of that, I think it's more "obviously correct" to do the full check here.
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1297598582)
I believe that's true, though only due to optimistic send itself (which guarantees that as soon as something is added to `vSendMsg`, it's immediately at least attempted to be moved to the transport). Without it, if there were two consecutive `PushMessage` without intervening `SocketSendData`, `vSendMsg` would already have something in it on the second one.
Because of that, I think it's more "obviously correct" to do the full check here.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1297627349)
Right, so I think `!node.m_sock` only happens when the node is already disconnected, or will never actually be connected to anything (in tests). I've added the following comment:
> There is no socket in case we've already disconnected, or in test cases without real connections. In these cases, we bail out immediately and just leave things in the send queue and transport.
Further up, above the `SetMessageToSend` call, I've added:
> This fails when there is an existing message still being
...
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1297627349)
Right, so I think `!node.m_sock` only happens when the node is already disconnected, or will never actually be connected to anything (in tests). I've added the following comment:
> There is no socket in case we've already disconnected, or in test cases without real connections. In these cases, we bail out immediately and just leave things in the send queue and transport.
Further up, above the `SetMessageToSend` call, I've added:
> This fails when there is an existing message still being
...
📝 brunoerg opened a pull request: "test: fix 'unknown named parameter' test in `wallet_basic`"
(https://github.com/bitcoin/bitcoin/pull/28288)
This PR removes loop when testing an unknown named parameter. They don't have any effect.
(https://github.com/bitcoin/bitcoin/pull/28288)
This PR removes loop when testing an unknown named parameter. They don't have any effect.
💬 brunoerg commented on pull request "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation":
(https://github.com/bitcoin/bitcoin/pull/28287#issuecomment-1682814806)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28287#issuecomment-1682814806)
Concept ACK
💬 AndriiHlukhovskyi commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1682836022)
Спасибо огромное очень познавательно я думаю что я тоже что то пропустил
!!! 🔥
чт, 17 авг. 2023 г., 12:58 mehran636311 ***@***.***>:
> Pellse full item my account open and stat
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1682073596>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BAMCSAPI2J4K356QRG4TQDLXVX2PDANCNFSM6AAAAAAW6JC3HY>
> .
> You are receiving this because you commented.M
...
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1682836022)
Спасибо огромное очень познавательно я думаю что я тоже что то пропустил
!!! 🔥
чт, 17 авг. 2023 г., 12:58 mehran636311 ***@***.***>:
> Pellse full item my account open and stat
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1682073596>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BAMCSAPI2J4K356QRG4TQDLXVX2PDANCNFSM6AAAAAAW6JC3HY>
> .
> You are receiving this because you commented.M
...
💬 sipa commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1297666902)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1297666902)
Fixed.
💬 sipa commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1297666973)
Done.
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1297666973)
Done.
💬 sipa commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1297667502)
Good catch, yes. I've added a commit that makes `ChaCha20::SetKey` wipe the buffer.
I don't think we should *in general* wipe the buffer whenever m_bufleft=0, but on a rekey operation it makes sense.
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1297667502)
Good catch, yes. I've added a commit that makes `ChaCha20::SetKey` wipe the buffer.
I don't think we should *in general* wipe the buffer whenever m_bufleft=0, but on a rekey operation it makes sense.
💬 sipa commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1682883296)
Addressed review comments by @stratospher. Note: new last commit.
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1682883296)
Addressed review comments by @stratospher. Note: new last commit.
💬 jonatack commented on pull request "test: Minor fix in test - locale in terminal":
(https://github.com/bitcoin/bitcoin/pull/28286#issuecomment-1682901139)
Concept ACK, though I'm unable to reproduce this on macOS, which makes sense per `src/common/system.cpp#L66-73`:
```cpp
// On most POSIX systems (e.g. Linux, but not BSD) the environment's locale
// may be invalid, in which case the "C.UTF-8" locale is used as fallback.
#if !defined(WIN32) && !defined(MAC_OSX) && !defined(__FreeBSD__) && !defined(__OpenBSD__) && !defined(__NetBSD__)
try {
std::locale(""); // Raises a runtime error if current locale is invalid
} c
...
(https://github.com/bitcoin/bitcoin/pull/28286#issuecomment-1682901139)
Concept ACK, though I'm unable to reproduce this on macOS, which makes sense per `src/common/system.cpp#L66-73`:
```cpp
// On most POSIX systems (e.g. Linux, but not BSD) the environment's locale
// may be invalid, in which case the "C.UTF-8" locale is used as fallback.
#if !defined(WIN32) && !defined(MAC_OSX) && !defined(__FreeBSD__) && !defined(__OpenBSD__) && !defined(__NetBSD__)
try {
std::locale(""); // Raises a runtime error if current locale is invalid
} c
...
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1297701571)
@sipa done! Thank you for the elegant solution.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1297701571)
@sipa done! Thank you for the elegant solution.
🤔 jonatack reviewed a pull request: "ci: Disable --coverage temporarily"
(https://github.com/bitcoin/bitcoin/pull/28285#pullrequestreview-1583409232)
ACK based on green CI
Perhaps the following would be better instead, so `test/functional/test_runner.py --coverage` run locally (as well as in the CI) doesn't needlessly report missing coverage.
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index d93e6fd6da2..14e3a140485 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -847,7 +576,7 @@ class RPCCoverage():
all_cmds = set()
# Consider RPC gene
...
(https://github.com/bitcoin/bitcoin/pull/28285#pullrequestreview-1583409232)
ACK based on green CI
Perhaps the following would be better instead, so `test/functional/test_runner.py --coverage` run locally (as well as in the CI) doesn't needlessly report missing coverage.
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index d93e6fd6da2..14e3a140485 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -847,7 +576,7 @@ class RPCCoverage():
all_cmds = set()
# Consider RPC gene
...
📝 mzumsande opened a pull request: "rpc: remove one more quote from non-string oneline description"
(https://github.com/bitcoin/bitcoin/pull/28289)
This fixes a silent conflict between https://github.com/bitcoin/bitcoin/pull/28123 (which removed all `\"options\"`) and https://github.com/bitcoin/bitcoin/pull/27460 (which added a new one).
It should fix the current CI failures.
(https://github.com/bitcoin/bitcoin/pull/28289)
This fixes a silent conflict between https://github.com/bitcoin/bitcoin/pull/28123 (which removed all `\"options\"`) and https://github.com/bitcoin/bitcoin/pull/27460 (which added a new one).
It should fix the current CI failures.
💬 mzumsande commented on pull request "ci: Disable --coverage temporarily":
(https://github.com/bitcoin/bitcoin/pull/28285#issuecomment-1682928094)
The root cause for the CI failures is silent merge conflict that should be fixed by #28289 (we might still disable the check though, no opinion on that).
(https://github.com/bitcoin/bitcoin/pull/28285#issuecomment-1682928094)
The root cause for the CI failures is silent merge conflict that should be fixed by #28289 (we might still disable the check though, no opinion on that).
💬 jonatack commented on issue "test: RPC coverage check doesn't work?":
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1682930814)
Verified the current CI issue (red on all pulls) locally
```bash
$ test/functional/test_runner.py --jobs=11 --timeout-factor=0 --coverage
.../...
Uncovered RPC commands:
- Internal
```
An initial fix
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index d93e6fd6da2..14e3a140485 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -847,7 +576,7 @@ class RPCCoverage():
all_cmds = set()
...
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1682930814)
Verified the current CI issue (red on all pulls) locally
```bash
$ test/functional/test_runner.py --jobs=11 --timeout-factor=0 --coverage
.../...
Uncovered RPC commands:
- Internal
```
An initial fix
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index d93e6fd6da2..14e3a140485 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -847,7 +576,7 @@ class RPCCoverage():
all_cmds = set()
...
💬 fjahr commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1297712132)
I see, I simply misread. I changed this to get the base path relative to file as a fallback. We could also simply use `.` or fall back to not setting a `cache_dir` at all but this seemed the most robust option to me.
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1297712132)
I see, I simply misread. I changed this to get the base path relative to file as a fallback. We could also simply use `.` or fall back to not setting a `cache_dir` at all but this seemed the most robust option to me.
🤔 jonatack reviewed a pull request: "rpc: remove one more quote from non-string oneline description"
(https://github.com/bitcoin/bitcoin/pull/28289#pullrequestreview-1583436980)
ACK 239431444216850b63ecf01c3b5c5d6d24230d08
With this change, `./test/functional/test_runner.py --jobs=11 --timeout-factor=0 --coverage` no longer warns as it does on current master:
```
Uncovered RPC commands:
- Internal
```
(https://github.com/bitcoin/bitcoin/pull/28289#pullrequestreview-1583436980)
ACK 239431444216850b63ecf01c3b5c5d6d24230d08
With this change, `./test/functional/test_runner.py --jobs=11 --timeout-factor=0 --coverage` no longer warns as it does on current master:
```
Uncovered RPC commands:
- Internal
```
💬 andrewtoth commented on pull request "validation: Flush state after initial sync":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1297745509)
Hmm it seems like this is the ordering for all methods in this class definition.
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1297745509)
Hmm it seems like this is the ordering for all methods in this class definition.