💬 ajtowns commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1683303626)
First commit from this has been merged via #28244; rebased on top of that. See #28285 or #28289 for CI failure.
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1683303626)
First commit from this has been merged via #28244; rebased on top of that. See #28285 or #28289 for CI failure.
📝 russeree converted_to_draft a pull request: "rpc: removed StrFormatInternalBug quote delimitation"
(https://github.com/bitcoin/bitcoin/pull/28291)
This PR rectifies an unnecessary set of quotes delimiting the contents of ```StrFormatInternalBug```. This is a follow up to @MarcoFalke https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1297191493. The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format.
The results can be seen below.
Previously
This PR rectifies an unnecessary set of quotes delimiting the contents of ```StrFormatInternalBug```. This is a follow up to @MarcoFalke https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1297191493. The method of action was to remove the escaped quotes that were a part of strprintf. A single functional test case was modified to reflect the new output format.
The results can be seen below.
Previously
![image](https://github.com/bitcoin/bitcoin/assets/3104223/53f9ea59-317f-4c62-
...
💬 stratospher commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1683310137)
ACK 57cc136.
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1683310137)
ACK 57cc136.
👋 russeree's pull request is ready for review: "rpc: removed StrFormatInternalBug quote delimitation"
(https://github.com/bitcoin/bitcoin/pull/28291)
(https://github.com/bitcoin/bitcoin/pull/28291)
💬 russeree commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1297992744)
Ref. https://github.com/bitcoin/bitcoin/pull/28291
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1297992744)
Ref. https://github.com/bitcoin/bitcoin/pull/28291
💬 ajtowns commented on pull request "rpc: remove one more quote from non-string oneline description":
(https://github.com/bitcoin/bitcoin/pull/28289#issuecomment-1683346423)
utACK 239431444216850b63ecf01c3b5c5d6d24230d08
(https://github.com/bitcoin/bitcoin/pull/28289#issuecomment-1683346423)
utACK 239431444216850b63ecf01c3b5c5d6d24230d08
💬 ajtowns 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-1683351664)
Concept ACK. Probably should add some basic functional tests to rpc_net.py with a `P2PInterface` checking that it works -- what happens if `msg_type` is 0 bytes, 13 bytes (`> protocol.h:COMMAND_SIZE`); what if `msg` is empty or more than 4MB (`net.h:MAX_PROTOCOL_MESSAGE_LENGTH`) or 32MB (`serialize.h:MAX_SIZE`)?
(https://github.com/bitcoin/bitcoin/pull/28287#issuecomment-1683351664)
Concept ACK. Probably should add some basic functional tests to rpc_net.py with a `P2PInterface` checking that it works -- what happens if `msg_type` is 0 bytes, 13 bytes (`> protocol.h:COMMAND_SIZE`); what if `msg` is empty or more than 4MB (`net.h:MAX_PROTOCOL_MESSAGE_LENGTH`) or 32MB (`serialize.h:MAX_SIZE`)?
💬 kevkevinpal commented on pull request "doc, refactor: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1298005740)
removed new line and updated title in [9a84200](https://github.com/bitcoin/bitcoin/pull/28101/commits/9a84200cfc994eebf38c46919b20e0c0261799ae)
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1298005740)
removed new line and updated title in [9a84200](https://github.com/bitcoin/bitcoin/pull/28101/commits/9a84200cfc994eebf38c46919b20e0c0261799ae)
💬 kevkevinpal commented on pull request "test: fix 'unknown named parameter' test in `wallet_basic`":
(https://github.com/bitcoin/bitcoin/pull/28288#issuecomment-1683375733)
ACK [d9ef627](https://github.com/bitcoin/bitcoin/pull/28288/commits/d9ef627af771987c134e72d8358798e6f376ffdf)
(https://github.com/bitcoin/bitcoin/pull/28288#issuecomment-1683375733)
ACK [d9ef627](https://github.com/bitcoin/bitcoin/pull/28288/commits/d9ef627af771987c134e72d8358798e6f376ffdf)
💬 MarcoFalke commented on pull request "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation":
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298027612)
```suggestion
thread1 = threading.Thread(target=node1.send_message, args=(0, "unknown", rand_msg))
```
Not sure if it works, but maybe you can drop the wrapper function?
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298027612)
```suggestion
thread1 = threading.Thread(target=node1.send_message, args=(0, "unknown", rand_msg))
```
Not sure if it works, but maybe you can drop the wrapper function?
💬 MarcoFalke commented on pull request "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation":
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298029632)
```suggestion
# Copyright (c) 2023-present The Bitcoin Core developers
```
nit: Either remove the typo, or remove the year, or append `present` to avoid having to "bump" this newly added file ever again.
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298029632)
```suggestion
# Copyright (c) 2023-present The Bitcoin Core developers
```
nit: Either remove the typo, or remove the year, or append `present` to avoid having to "bump" this newly added file ever again.
💬 MarcoFalke commented on pull request "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation":
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298033278)
nit: Clang-format-diff new code?
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298033278)
nit: Clang-format-diff new code?
💬 MarcoFalke commented on pull request "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation":
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298032250)
```suggestion
"Send a p2p message to a peer specified by id.\n"
"The message type and body must be provided, the message header will be generated.\n"
"This RPC is for testing only.",
```
nit
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298032250)
```suggestion
"Send a p2p message to a peer specified by id.\n"
"The message type and body must be provided, the message header will be generated.\n"
"This RPC is for testing only.",
```
nit
💬 MarcoFalke commented on pull request "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation":
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298031624)
nit: Use `TryParseHex`?
(https://github.com/bitcoin/bitcoin/pull/28287#discussion_r1298031624)
nit: Use `TryParseHex`?
👍 MarcoFalke approved a pull request: "rpc, test: add `sendmsgtopeer` rpc and a test for net-level deadlock situation"
(https://github.com/bitcoin/bitcoin/pull/28287#pullrequestreview-1583884554)
lgtm. Feel free to ignore the nits.
(https://github.com/bitcoin/bitcoin/pull/28287#pullrequestreview-1583884554)
lgtm. Feel free to ignore the nits.
💬 MarcoFalke 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-1683390638)
`Error: RPC command "sendmsgtopeer" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.`
(https://github.com/bitcoin/bitcoin/pull/28287#issuecomment-1683390638)
`Error: RPC command "sendmsgtopeer" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.`
💬 MarcoFalke commented on pull request "rpc: remove one more quote from non-string oneline description":
(https://github.com/bitcoin/bitcoin/pull/28289#issuecomment-1683391746)
lgtm ACK 239431444216850b63ecf01c3b5c5d6d24230d08
(https://github.com/bitcoin/bitcoin/pull/28289#issuecomment-1683391746)
lgtm ACK 239431444216850b63ecf01c3b5c5d6d24230d08
✅ MarcoFalke closed a pull request: "ci: Disable --coverage temporarily"
(https://github.com/bitcoin/bitcoin/pull/28285)
(https://github.com/bitcoin/bitcoin/pull/28285)
💬 MarcoFalke commented on pull request "rpc: removed StrFormatInternalBug quote delimitation":
(https://github.com/bitcoin/bitcoin/pull/28291#issuecomment-1683400075)
review ACK 6e8f6468cbf1320b70cf01333002a31b44cb7c33
Makes sense to use a single function consistently. This will now also print `file, line, func` of where the *check* is located, not where the presumed *bug* is located, but this seems fine, because the bug could also be in the check. In theory, there could be a flag to disable `file, line, func`, but that seems over-optimizing for developer-only code paths that are rarely hit.
(https://github.com/bitcoin/bitcoin/pull/28291#issuecomment-1683400075)
review ACK 6e8f6468cbf1320b70cf01333002a31b44cb7c33
Makes sense to use a single function consistently. This will now also print `file, line, func` of where the *check* is located, not where the presumed *bug* is located, but this seems fine, because the bug could also be in the check. In theory, there could be a flag to disable `file, line, func`, but that seems over-optimizing for developer-only code paths that are rarely hit.
📝 hebasto opened a pull request: "ci: Disable cache save for pull requests in GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28292)
This PR disable cache save for pull requests in GitHub Actions.
Otherwise, multiple pull requests fill GitHub Actions cache quota shortly.
See a discussion [here](https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1295459732).
---
**NOTE** for the maintainers with "owner" permissions.
This PR needs the `actions/cache/restore@*` and `actions/cache/save@*` acrions to be explicitly allowed in the repository's Actions permissions.
(https://github.com/bitcoin/bitcoin/pull/28292)
This PR disable cache save for pull requests in GitHub Actions.
Otherwise, multiple pull requests fill GitHub Actions cache quota shortly.
See a discussion [here](https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1295459732).
---
**NOTE** for the maintainers with "owner" permissions.
This PR needs the `actions/cache/restore@*` and `actions/cache/save@*` acrions to be explicitly allowed in the repository's Actions permissions.