💬 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.
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269027565)
I will look into replacing this with `Assert()`.
"exception from destructor in c++" is a topic well covered all over the internet. The compiler produces a warning for that. Here is an example:
```cpp
class A
{
public:
~A() {
std::cout << "before throw a\n";
// warning: '~A' has a non-throwing exception specification but can still throw [-Wexceptions]
//171 | throw std::runtime_error("a");
// | ^
throw std::runtime_er
...
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269027565)
I will look into replacing this with `Assert()`.
"exception from destructor in c++" is a topic well covered all over the internet. The compiler produces a warning for that. Here is an example:
```cpp
class A
{
public:
~A() {
std::cout << "before throw a\n";
// warning: '~A' has a non-throwing exception specification but can still throw [-Wexceptions]
//171 | throw std::runtime_error("a");
// | ^
throw std::runtime_er
...
💬 MarcoFalke commented on pull request "fuzz: Generate process_message targets individually":
(https://github.com/bitcoin/bitcoin/pull/28066#issuecomment-1643403050)
Anything left to do for a test-only change with two ACKs or is this rfm?
(https://github.com/bitcoin/bitcoin/pull/28066#issuecomment-1643403050)
Anything left to do for a test-only change with two ACKs or is this rfm?
💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269042432)
Yeah, but this being an "Abort trap" is well defined, not "not allowed". And I think the tests aborting on failure is fine.
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269042432)
Yeah, but this being an "Abort trap" is well defined, not "not allowed". And I think the tests aborting on failure is fine.
✅ hebasto closed a pull request: "Add `UNREACHABLE` macro"
(https://github.com/bitcoin/bitcoin/pull/26504)
(https://github.com/bitcoin/bitcoin/pull/26504)
⚠️ hebasto opened an issue: "build: Windows debug cross-build fails with `-O0`"
(https://github.com/bitcoin/bitcoin/issues/28109)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When cross-compiling for Windows with the `-O0` optimization level the build fails.
The error can be fixed by the `-Wa,-mbig-obj` assembler flag. However, @theuni [suggested](https://github.com/hebasto/bitcoin/pull/18#discussion_r1267219453):
> We should definitely look into breaking up that object.
### Expected behaviour
The build completes with no error.
### Steps to reproduce
``
...
(https://github.com/bitcoin/bitcoin/issues/28109)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When cross-compiling for Windows with the `-O0` optimization level the build fails.
The error can be fixed by the `-Wa,-mbig-obj` assembler flag. However, @theuni [suggested](https://github.com/hebasto/bitcoin/pull/18#discussion_r1267219453):
> We should definitely look into breaking up that object.
### Expected behaviour
The build completes with no error.
### Steps to reproduce
``
...
✅ fanquake closed an issue: "bumpfee behavior with custom change address"
(https://github.com/bitcoin/bitcoin/issues/11233)
(https://github.com/bitcoin/bitcoin/issues/11233)