📝 mzumsande opened a pull request: "test: fix intermittent failure in wallet_resendwallettransactions.py"
(https://github.com/bitcoin/bitcoin/pull/28108)
Fixes #28094
The test bumps the mocktime for ~2 weeks and then triggers eviction from the mempool. But this bump will also cause a new resubmit, and if the timing is such that this resubmit happens right after the eviction and before the check that the tx was evicted, the test can fail as in #28094:
```
node0 2023-07-17T21:31:23.809483Z (mocktime: 2023-08-02T09:46:27Z) [httpworker.1] [validation.cpp:267] [LimitMempoolSize] [mempool] Expired 2 transactions from the memory pool
node0 2023-
...
(https://github.com/bitcoin/bitcoin/pull/28108)
Fixes #28094
The test bumps the mocktime for ~2 weeks and then triggers eviction from the mempool. But this bump will also cause a new resubmit, and if the timing is such that this resubmit happens right after the eviction and before the check that the tx was evicted, the test can fail as in #28094:
```
node0 2023-07-17T21:31:23.809483Z (mocktime: 2023-08-02T09:46:27Z) [httpworker.1] [validation.cpp:267] [LimitMempoolSize] [mempool] Expired 2 transactions from the memory pool
node0 2023-
...
👍 stickies-v approved a pull request: "net processing, refactor: Decouple PeerManager from gArgs"
(https://github.com/bitcoin/bitcoin/pull/27499#pullrequestreview-1537528603)
ACK 064ac73d0715fb2c371e68f8d4d234fee002299b
(https://github.com/bitcoin/bitcoin/pull/27499#pullrequestreview-1537528603)
ACK 064ac73d0715fb2c371e68f8d4d234fee002299b
💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268348590)
nit: no longer needed
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268348590)
nit: no longer needed
💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268347744)
nit: with this change, `#include <node/txreconciliation.h>` can be removed from `init.cpp`
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268347744)
nit: with this change, `#include <node/txreconciliation.h>` can be removed from `init.cpp`
💬 stickies-v commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386)
nit: probably best to use `DEFAULT_BLOCKSONLY` to make it explicit what `false` refers to, and keep it in sync?
```suggestion
bool ignore_incoming_txs{DEFAULT_BLOCKSONLY};
```
Alternatively (but I don't think using `peerman_opts` like this is elegant), could deduplicate it entirely:
<details>
<summary>git diff on e3ddc5cc96cf9f6339e7828c570fa5ad5130918a</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index dd7a359a2..8b62690f7 100644
--- a/src/init.cpp
+++ b/s
...
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386)
nit: probably best to use `DEFAULT_BLOCKSONLY` to make it explicit what `false` refers to, and keep it in sync?
```suggestion
bool ignore_incoming_txs{DEFAULT_BLOCKSONLY};
```
Alternatively (but I don't think using `peerman_opts` like this is elegant), could deduplicate it entirely:
<details>
<summary>git diff on e3ddc5cc96cf9f6339e7828c570fa5ad5130918a</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index dd7a359a2..8b62690f7 100644
--- a/src/init.cpp
+++ b/s
...
🤔 jonatack reviewed a pull request: "test: fix intermittent failure in wallet_resendwallettransactions.py"
(https://github.com/bitcoin/bitcoin/pull/28108#pullrequestreview-1537849103)
Light "this looks like the other tests in this file" ACK e667bd68a10512ddc784df44bdcb63ee441e5551
(https://github.com/bitcoin/bitcoin/pull/28108#pullrequestreview-1537849103)
Light "this looks like the other tests in this file" ACK e667bd68a10512ddc784df44bdcb63ee441e5551
💬 jonatack commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#discussion_r1268555995)
<details><summary>Some doc and logging ideas after a cursory look, feel free to ignore</summary><p>
```diff
@@ -109,21 +109,22 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
node.syncwithvalidationinterfacequeue()
evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5
- # Flush out currently scheduled resubmit attempt now so that there can't be one right between eviction and check.
+
+ self.log.info("Flush out currently sche
...
(https://github.com/bitcoin/bitcoin/pull/28108#discussion_r1268555995)
<details><summary>Some doc and logging ideas after a cursory look, feel free to ignore</summary><p>
```diff
@@ -109,21 +109,22 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
node.syncwithvalidationinterfacequeue()
evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5
- # Flush out currently scheduled resubmit attempt now so that there can't be one right between eviction and check.
+
+ self.log.info("Flush out currently sche
...
💬 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
...