🚀 fanquake merged a pull request: "test: Restore unlimited timeout in IndexWaitSynced"
(https://github.com/bitcoin/bitcoin/pull/28036)
(https://github.com/bitcoin/bitcoin/pull/28036)
👍 hebasto approved a pull request: "wallet: don't include bdb files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28039#pullrequestreview-1518398328)
Approach ACK 2d09d0f50408f9c522b9efa4f386072502c7b3d1.
Suggesting to adjust our IWYU mapping file:
```diff
--- a/contrib/devtools/iwyu/bitcoin.core.imp
+++ b/contrib/devtools/iwyu/bitcoin.core.imp
@@ -4,4 +4,7 @@
{ include: [ "<bits/termios-struct.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/termios-tcflow.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/chrono.h>", private, "<chrono>", public ] },
+
+ # Berkeley DB
+ { include: [ "<db.h>", pri
...
(https://github.com/bitcoin/bitcoin/pull/28039#pullrequestreview-1518398328)
Approach ACK 2d09d0f50408f9c522b9efa4f386072502c7b3d1.
Suggesting to adjust our IWYU mapping file:
```diff
--- a/contrib/devtools/iwyu/bitcoin.core.imp
+++ b/contrib/devtools/iwyu/bitcoin.core.imp
@@ -4,4 +4,7 @@
{ include: [ "<bits/termios-struct.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/termios-tcflow.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/chrono.h>", private, "<chrono>", public ] },
+
+ # Berkeley DB
+ { include: [ "<db.h>", pri
...
💬 MarcoFalke commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1625108039)
Is this rfm?
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1625108039)
Is this rfm?
🚀 fanquake merged a pull request: "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization"
(https://github.com/bitcoin/bitcoin/pull/28012)
(https://github.com/bitcoin/bitcoin/pull/28012)
💬 hebasto commented on pull request "bugfix: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1625154134)
Rebased e89273e80765219ff545ee5c26a257aef7d69f9c -> b7ebc999ee373f0429939ac0bdb8e38fb37404aa ([pr26762.11](https://github.com/hebasto/bitcoin/commits/pr26762.11) -> [pr26762.12](https://github.com/hebasto/bitcoin/commits/pr26762.12)) due to the conflict with #27861.
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1625154134)
Rebased e89273e80765219ff545ee5c26a257aef7d69f9c -> b7ebc999ee373f0429939ac0bdb8e38fb37404aa ([pr26762.11](https://github.com/hebasto/bitcoin/commits/pr26762.11) -> [pr26762.12](https://github.com/hebasto/bitcoin/commits/pr26762.12)) due to the conflict with #27861.
💬 willcl-ark commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1625181541)
> > This has the effect on this PR that many of the every~5 minute network specific connections are wasted on ipv6 attempts.
>
> completely related, thanks for sharing! this makes sense based on the current implementation. the original proposal had special case logic to prevent this situation, but the direction of review has led to deciding this is an acceptable tradeoff. incase helpful, [#27213 (comment)](https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1577424366) provides a recap
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1625181541)
> > This has the effect on this PR that many of the every~5 minute network specific connections are wasted on ipv6 attempts.
>
> completely related, thanks for sharing! this makes sense based on the current implementation. the original proposal had special case logic to prevent this situation, but the direction of review has led to deciding this is an acceptable tradeoff. incase helpful, [#27213 (comment)](https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1577424366) provides a recap
...
🚀 fanquake merged a pull request: "fuzz: Generate rpc fuzz targets individually"
(https://github.com/bitcoin/bitcoin/pull/28015)
(https://github.com/bitcoin/bitcoin/pull/28015)
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255585328)
Is adding the txid still necessary though?
IIUC, the purpose of adding this transaction's txid to `inventory_known_filter` is to save a step if this transaction has an unconfirmed child (if it already existed in this filter, then we wouldn't add it to the `recently_announced` filter). The PR is deleting those lines from `ProcessGetData`, and that's fine since an unconfirmed parent's sequence number should before that of its child.
But I'm not able to see another reason why we should add the
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255585328)
Is adding the txid still necessary though?
IIUC, the purpose of adding this transaction's txid to `inventory_known_filter` is to save a step if this transaction has an unconfirmed child (if it already existed in this filter, then we wouldn't add it to the `recently_announced` filter). The PR is deleting those lines from `ProcessGetData`, and that's fine since an unconfirmed parent's sequence number should before that of its child.
But I'm not able to see another reason why we should add the
...
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255631372)
```suggestion
/** Target number of transaction inventory items to send per transmission. */
```
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255631372)
```suggestion
/** Target number of transaction inventory items to send per transmission. */
```
💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255524853)
I think this just refers to external to the *mempool*.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255524853)
I think this just refers to external to the *mempool*.
📝 dergoegge opened a pull request: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043)
This PR adds a regression fuzz test for #26355 and [some of the bugs](https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990) found during review of #25717.
Should give us more confidence in doing #25725.
Depends on #27499.
(https://github.com/bitcoin/bitcoin/pull/28043)
This PR adds a regression fuzz test for #26355 and [some of the bugs](https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990) found during review of #25717.
Should give us more confidence in doing #25725.
Depends on #27499.
📝 dergoegge converted_to_draft a pull request: "fuzz: Test headers pre-sync through p2p interface"
(https://github.com/bitcoin/bitcoin/pull/28043)
This PR adds a regression fuzz test for #26355 and [some of the bugs](https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990) found during review of #25717.
Should give us more confidence in doing #25725.
Depends on #27499.
(https://github.com/bitcoin/bitcoin/pull/28043)
This PR adds a regression fuzz test for #26355 and [some of the bugs](https://github.com/bitcoin/bitcoin/pull/25717/commits/ed6cddd98e32263fc116a4380af6d66da20da990) found during review of #25717.
Should give us more confidence in doing #25725.
Depends on #27499.
💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1625315268)
Draft until #27499 is in.
(https://github.com/bitcoin/bitcoin/pull/28043#issuecomment-1625315268)
Draft until #27499 is in.
👍 dergoegge approved a pull request: "ci: build Valgrind (3.21) from source"
(https://github.com/bitcoin/bitcoin/pull/27992#pullrequestreview-1518811424)
utACK 83266cd4b5cb419a4ba45d4a55b955d6a4c82cd7
(https://github.com/bitcoin/bitcoin/pull/27992#pullrequestreview-1518811424)
utACK 83266cd4b5cb419a4ba45d4a55b955d6a4c82cd7
💬 furszy commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625395662)
> Does your feedback from [#28036 (comment)](https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703) also apply here? I think you'll also have to check for it being synced?
>
> ```
> Assert(index.GetSummary().synced);
> ```
Yeah @MarcoFalke. Same applies here. I didn't push it just because of #28036. Thought that we were going to tackle it there.
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625395662)
> Does your feedback from [#28036 (comment)](https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703) also apply here? I think you'll also have to check for it being synced?
>
> ```
> Assert(index.GetSummary().synced);
> ```
Yeah @MarcoFalke. Same applies here. I didn't push it just because of #28036. Thought that we were going to tackle it there.
📝 furszy opened a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044)
Coming from https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703, I thought that we were going to fix it there but seems that got merged without it for some reason.
As index sync failures trigger a shutdown request without notifying `BaseIndex::BlockUntilSyncedToCurrentChain` in any way, we also need to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test forever.
(https://github.com/bitcoin/bitcoin/pull/28044)
Coming from https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703, I thought that we were going to fix it there but seems that got merged without it for some reason.
As index sync failures trigger a shutdown request without notifying `BaseIndex::BlockUntilSyncedToCurrentChain` in any way, we also need to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test forever.
💬 jonatack commented on issue "I2P: Creating SAM session with 127.0.0.1:7656":
(https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1625409479)
IRC discussion today about this issue: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-07-07#941511
(https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1625409479)
IRC discussion today about this issue: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-07-07#941511
💬 MarcoFalke commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625411428)
Looks like https://github.com/bitcoin/bitcoin/pull/28036 got merged in the meantime, so I'll add the `Assert(!ShutdownRequested())` as follow-up on Monday unless someone beats me to it.
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625411428)
Looks like https://github.com/bitcoin/bitcoin/pull/28036 got merged in the meantime, so I'll add the `Assert(!ShutdownRequested())` as follow-up on Monday unless someone beats me to it.
💬 furszy commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625413440)
> Looks like #28036 got merged in the meantime, so I'll add the `Assert(!ShutdownRequested())` as follow-up on Monday unless someone beats me to it.
Yeah, not sure why it got merged it. Already pushed #28044 fixing it.
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625413440)
> Looks like #28036 got merged in the meantime, so I'll add the `Assert(!ShutdownRequested())` as follow-up on Monday unless someone beats me to it.
Yeah, not sure why it got merged it. Already pushed #28044 fixing it.
👍 MarcoFalke approved a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044#pullrequestreview-1518886109)
lgtm
(https://github.com/bitcoin/bitcoin/pull/28044#pullrequestreview-1518886109)
lgtm