💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1642518571)
Updated to take all feedback by @stickies-v and @vasild -- thank you!
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1642518571)
Updated to take all feedback by @stickies-v and @vasild -- thank you!
💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1268486082)
> Done, changed from implicit in the declarations to explicit in the definitions.
The CI is unhappy with that, so reverting the last commit to c69a74229da514228d3388e9652d2ea2e89224d7.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1268486082)
> Done, changed from implicit in the declarations to explicit in the definitions.
The CI is unhappy with that, so reverting the last commit to c69a74229da514228d3388e9652d2ea2e89224d7.
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1642547623)
ACK ba8ab4fc545800c4fb283a5ff0b19138a0451aba
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1642547623)
ACK ba8ab4fc545800c4fb283a5ff0b19138a0451aba
📝 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.