💬 ryanofsky commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1642661487)
This PR has two stale acks and seems like a safe documentation change. I think it would be good to merge if original reviewers want to reack or anyone else wants to review.
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1642661487)
This PR has two stale acks and seems like a safe documentation change. I think it would be good to merge if original reviewers want to reack or anyone else wants to review.
💬 achow101 commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#issuecomment-1642698705)
ACK 7d92b1430a6fd42c4438810640576830d0ff8d13
(https://github.com/bitcoin/bitcoin/pull/28085#issuecomment-1642698705)
ACK 7d92b1430a6fd42c4438810640576830d0ff8d13
🚀 achow101 merged a pull request: "refactor: use Span for SipHash::Write"
(https://github.com/bitcoin/bitcoin/pull/28085)
(https://github.com/bitcoin/bitcoin/pull/28085)
🚀 ryanofsky merged a pull request: "test: Add more tests for the BIP21 implementation"
(https://github.com/bitcoin/bitcoin/pull/27928)
(https://github.com/bitcoin/bitcoin/pull/27928)
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268667959)
Ah thanks, that belongs to the commit one later, where we need the chain interface to amend the bump fees in case of overlapping ancestries.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268667959)
Ah thanks, that belongs to the commit one later, where we need the chain interface to amend the bump fees in case of overlapping ancestries.
💬 kristapsk commented on pull request "test: Add more tests for the BIP21 implementation":
(https://github.com/bitcoin/bitcoin/pull/27928#discussion_r1268722215)
It this correct? I mean, according to BIP21, not existing code. BIP21 specifies it as "valid characters of an RFC 3986 URI query component, excluding the "=" and "&" characters" and %xx should be decoded according to RFC 3986. That should be only way how to present "=" and "&" in message and label.
(https://github.com/bitcoin/bitcoin/pull/27928#discussion_r1268722215)
It this correct? I mean, according to BIP21, not existing code. BIP21 specifies it as "valid characters of an RFC 3986 URI query component, excluding the "=" and "&" characters" and %xx should be decoded according to RFC 3986. That should be only way how to present "=" and "&" in message and label.
💬 kristapsk commented on pull request "test: Add more tests for the BIP21 implementation":
(https://github.com/bitcoin/bitcoin/pull/27928#discussion_r1268735818)
Guess should also check the case when the first amount value is invalid, second valid. Should it be considered valid or invalid URI then?
(https://github.com/bitcoin/bitcoin/pull/27928#discussion_r1268735818)
Guess should also check the case when the first amount value is invalid, second valid. Should it be considered valid or invalid URI then?
💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268769040)
Won't the second `emplace` just do nothing both if both `select_send` and `select_recv` are true? I thought the idea was to change behavior to have both send and recv requested events (instead of just giving `select_send` priority like in master). But wouldn't we need to insert a combination of `Sock::SEND` and `Sock::RECV` then, instead of repeated `emplace`?
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268769040)
Won't the second `emplace` just do nothing both if both `select_send` and `select_recv` are true? I thought the idea was to change behavior to have both send and recv requested events (instead of just giving `select_send` priority like in master). But wouldn't we need to insert a combination of `Sock::SEND` and `Sock::RECV` then, instead of repeated `emplace`?
🤔 jonatack reviewed a pull request: "refactor: extract CCheckQueue's data handling into a separate container "Bag""
(https://github.com/bitcoin/bitcoin/pull/27331#pullrequestreview-1538190456)
Approach ACK 6b0537c4a71fe774ea12c097d5c16dc2a8ebb0a2
Needs trivial rebase, an adjacent Makefile entry and include header.
(https://github.com/bitcoin/bitcoin/pull/27331#pullrequestreview-1538190456)
Approach ACK 6b0537c4a71fe774ea12c097d5c16dc2a8ebb0a2
Needs trivial rebase, an adjacent Makefile entry and include header.
💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268782895)
The order, or splitting, does not matter. All of these are fed to `Sock::WaitMany`, which will mark the ones that are ready for sending to/receiving from. It's in the processing of those wait results that the prioritization happens, where receiving is skipped if (a) the socket was ready for sending (b) something was sent and (c) there is yet more to send.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268782895)
The order, or splitting, does not matter. All of these are fed to `Sock::WaitMany`, which will mark the ones that are ready for sending to/receiving from. It's in the processing of those wait results that the prioritization happens, where receiving is skipped if (a) the socket was ready for sending (b) something was sent and (c) there is yet more to send.
💬 mzumsande commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268852909)
I'm still confused: `events_per_sock` is a `std::unordered_map`. If `select_send` and `select_recv` are true, we now `emplace` twice into it, with the same key `pnode->m_sock` and different values. That means that the first value stays, and the second `emplace` is a no-op, leaving the container unchanged. So if only the first value (in this case `Sock::SEND`) is fed to `Sock::WaitMany`, why doesn't the order matter?
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268852909)
I'm still confused: `events_per_sock` is a `std::unordered_map`. If `select_send` and `select_recv` are true, we now `emplace` twice into it, with the same key `pnode->m_sock` and different values. That means that the first value stays, and the second `emplace` is a no-op, leaving the container unchanged. So if only the first value (in this case `Sock::SEND`) is fed to `Sock::WaitMany`, why doesn't the order matter?
💬 sipa commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268854292)
Oh, I had missed the data type, and thought you were talking something else.
You're absolutely right, will address.
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268854292)
Oh, I had missed the data type, and thought you were talking something else.
You're absolutely right, will address.
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268855297)
I think the concern here is that `events_per_sock` is a `Sock::EventsPerSock` which is an unordered map, so using the same key in two `emplace` calls would cause the second to overwrite the first. The key type here is `shared_ptr<const Sock>` and the `std::hash` for shared ptrs just looks at the address they're pointing too, so as far as I can see the second emplace here will indeed overwrite the first, making this effectively the same as:
```
if (select_recv) {
emplace(RECV);
} else i
...
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1268855297)
I think the concern here is that `events_per_sock` is a `Sock::EventsPerSock` which is an unordered map, so using the same key in two `emplace` calls would cause the second to overwrite the first. The key type here is `shared_ptr<const Sock>` and the `std::hash` for shared ptrs just looks at the address they're pointing too, so as far as I can see the second emplace here will indeed overwrite the first, making this effectively the same as:
```
if (select_recv) {
emplace(RECV);
} else i
...
💬 MarcoFalke commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1643269814)
Nice. lgtm ACK e667bd68a10512ddc784df44bdcb63ee441e5551
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1643269814)
Nice. lgtm ACK e667bd68a10512ddc784df44bdcb63ee441e5551
🤔 MarcoFalke reviewed a pull request: "test: fix intermittent failure in wallet_resendwallettransactions.py"
(https://github.com/bitcoin/bitcoin/pull/28108#pullrequestreview-1538476086)
left a follow-up idea
(https://github.com/bitcoin/bitcoin/pull/28108#pullrequestreview-1538476086)
left a follow-up idea
💬 MarcoFalke commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#discussion_r1268966663)
Generally I am not really a fan of using `assert_debug_log` to sync. If you do, my preference would be to use the timeout argument. Even better would be to sync inside the body. Either with a call to `syncwithvalidationinterfaceque` or by modifying the RPC:
```diff
diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp
index 3828401642..f614253195 100644
--- a/src/rpc/node.cpp
+++ b/src/rpc/node.cpp
@@ -93,6 +93,7 @@ static RPCHelpMan mockscheduler()
// protect against null pointer der
...
(https://github.com/bitcoin/bitcoin/pull/28108#discussion_r1268966663)
Generally I am not really a fan of using `assert_debug_log` to sync. If you do, my preference would be to use the timeout argument. Even better would be to sync inside the body. Either with a call to `syncwithvalidationinterfaceque` or by modifying the RPC:
```diff
diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp
index 3828401642..f614253195 100644
--- a/src/rpc/node.cpp
+++ b/src/rpc/node.cpp
@@ -93,6 +93,7 @@ static RPCHelpMan mockscheduler()
// protect against null pointer der
...
💬 MarcoFalke commented on pull request "util: Show descriptive error messages when FileCommit fails":
(https://github.com/bitcoin/bitcoin/pull/26654#issuecomment-1643358133)
CI can be ignored.
lgtm ACK 5408a55fc87350baeae3a32f1003f956d5533a79 💡
<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: lgt
...
(https://github.com/bitcoin/bitcoin/pull/26654#issuecomment-1643358133)
CI can be ignored.
lgtm ACK 5408a55fc87350baeae3a32f1003f956d5533a79 💡
<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: lgt
...
💬 MarcoFalke commented on pull request "refactor: Open file in FileCommit":
(https://github.com/bitcoin/bitcoin/pull/28006#issuecomment-1643368606)
Looks like this will make it safer to remove `std::ftell` when using XOR, so I'll reopen for now as draft.
(https://github.com/bitcoin/bitcoin/pull/28006#issuecomment-1643368606)
Looks like this will make it safer to remove `std::ftell` when using XOR, so I'll reopen for now as draft.
📝 MarcoFalke reopened a pull request: "refactor: Open file in FileCommit"
(https://github.com/bitcoin/bitcoin/pull/28006)
This improves `FileCommit`: The patch removes the requirement from the call site to open a raw C-style FILE pointer. Now the call site can use any style of file IO.
There are many other smaller improvements, which are explained in each commit.
(https://github.com/bitcoin/bitcoin/pull/28006)
This improves `FileCommit`: The patch removes the requirement from the call site to open a raw C-style FILE pointer. Now the call site can use any style of file IO.
There are many other smaller improvements, which are explained in each commit.
📝 MarcoFalke converted_to_draft a pull request: "refactor: Open file in FileCommit"
(https://github.com/bitcoin/bitcoin/pull/28006)
This improves `FileCommit`: The patch removes the requirement from the call site to open a raw C-style FILE pointer. Now the call site can use any style of file IO.
There are many other smaller improvements, which are explained in each commit.
(https://github.com/bitcoin/bitcoin/pull/28006)
This improves `FileCommit`: The patch removes the requirement from the call site to open a raw C-style FILE pointer. Now the call site can use any style of file IO.
There are many other smaller improvements, which are explained in each commit.