💬 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*.

📝 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.

📝 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.

💬 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.

👍 dergoegge approved a pull request: "ci: build Valgrind (3.21) from source"
(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.

📝 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.

💬 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

💬 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.

💬 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.

👍 MarcoFalke approved a pull request: "test: indexes, fix on error infinite loop"
(https://github.com/bitcoin/bitcoin/pull/28044#pullrequestreview-1518886109)
lgtm

💬 MarcoFalke commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#discussion_r1255822846)
I think you can just check `Assert(!ShutdownRequested())` inside the loop, to give a better error message when an internal error causes a shutdown, instead of breaking out of the loop and then asserting `!synced`?

💬 MarcoFalke commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625418466)
Thanks, I think this one can be closed for now?

✅ furszy closed a pull request: "test: bugfix, synchronize indexes synchronously"
(https://github.com/bitcoin/bitcoin/pull/28026)

💬 fanquake commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625419951)
> Already pushed https://github.com/bitcoin/bitcoin/pull/28044 fixing it.

Why not just push it here, and re-use this PR (dropping any now-redundant changes) . Otherwise, why is this still open.

> Yeah, not sure why it got merged it.

It got merged to fix the persistent (potentially confusing) CI issues. What you want to fix isn't currently an active issue.

📝 dergoegge opened a pull request: "rfc: Nuke getblocks message"
(https://github.com/bitcoin/bitcoin/pull/28045)
Is this message still needed, given that we have had headers first sync for a **very** long time (https://github.com/bitcoin/bitcoin/pull/4468)?

💬 sipa commented on pull request "rfc: Nuke getblocks message":
(https://github.com/bitcoin/bitcoin/pull/28045#issuecomment-1625436903)
Yes, not every P2P client uses headers-first sync.

💬 furszy commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1625445909)
> Why not just push it here, and re-use this PR (dropping any now-redundant changes) . Otherwise, why is this still open.

Closed the PR a minute before your comment.

> It got merged to fix the persistent (potentially confusing) CI issues. What you want to fix isn't currently an active issue.

Well, the previous CI failure was due a pretty clear timeout error. Now there is a possibility of tests hanging forever with no clear reason. So, this last one is practically more confusing than the
...

💬 theStack commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255855804)
Changed to use the comparison operator. Note that the "can't go below zero-fee" comment from the above was removed, so I'm also not using it here.

💬 theStack commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255855901)
Agree, done.