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

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

💬 theStack commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255856084)
Agree, done. Note that I had to trigger an UTXO set rescan for MiniWallet after restart, otherwise it would sometimes try to spend UTXOs from mempool transactions (which are now not available anymore).

💬 theStack commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255856373)
You're right. I think the "disabled" string originated from sloppily copy/pasting the test loop structure from mempool_dust.py. Replaced by "zero" now.

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255860737)
Ah good thinking!

💬 theStack commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255861498)
The idea was to check that in both RPC code-paths that result in assembling blocks (`generateto*` and `getblocktemplate`, AFAICT there aren't any others), the setting has the same effect. Happy to remove one of them though, if that's seen as redundant.

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255862835)
No need for this condition anymore
```suggestion
assert tx_below_min_feerate['txid'] not in block_template_txids
assert tx_below_min_feerate['txid'] not in block_txids
```

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

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255867530)
No need to remove, I was just wondering

💬 furszy commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#discussion_r1255877236)
pushed.

💬 0xB10C commented on pull request "rfc: Nuke getblocks message":
(https://github.com/bitcoin/bitcoin/pull/28045#issuecomment-1625470129)
I have a `-peerbloomfilters=1` node that receives a few `getblocks` messages per minute. The peers have `/BitcoinKit:0.1.0/`, /WalletKit:0.1.0/`, `/bread:2.1/` as user-agents.

💬 MarcoFalke commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#issuecomment-1625471588)
Nice, thank you.

lgtm ACK 4b405318d4c476351cccc30588f9126edd94cc35