🤔 Crypt-iQ reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3393285716)
I agree with @dergoegge that it's easy to become a HB peer if you're already the first to send unsolicited compact blocks to try and fingerprint a node's mempool. I think the benefit with this change is being more strict with what we'll accept and following the protocol closer (and I think there's even more improvements we can make). Left some test-related nits.

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2473220795)
nit: can remove this line, `stalling_peer` is already HB

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2472976009)
These two lines can be removed if `self.segwit_node` is marked as HB above?

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2473738447)
Future improvements to fully "lock down" this code path could be to:
- check `m_opts.ignore_incoming_txs`
- check `m_provides_cmpctblocks` is set
- check that if the block was requested, we used `MSG_CMPCT_BLOCK` (requires state)

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2472970994)
Since `self.segwit_node` was disconnected in the prior test it's not yet HB, plus it doesn't look like it has sent over `sendcmpct` yet. I agree that we should assert that it is marked as HB though.

💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2473635706)
`unsolicited_peer` also hasn't sent `sendcmpct` so its `m_provides_cmpctblocks` is false, and the node still processes the compact block! I think it is useful to have coverage for this case.

👍 pinheadmz approved a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3385232782)
ACK 195a96258f970c384ce180d57e73616904ef5fa1

Built and tested on macos/arm64 and debian/x86. Reviewed each commit and left a few comments.

I also tested the branch against other popular software that consumes the HTTP interface by running their CI:
- [bitcoinjs-lib](https://github.com/pinheadmz/bitcoinjs-lib/pull/2)
- [rpc-bitcoin](https://github.com/pinheadmz/rpc-bitcoin/pull/2)
- [eclair](https://github.com/pinheadmz/eclair/pull/2)
- [lnd](https://github.com/pinheadmz/lnd/pull/2/)
- [electrs
...

💬 pinheadmz commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469913117)
c219b93c3b043de202bdf3c65b433fd17af2da89

I was wondering if there's any way to assert that the unblocked tasks are all executing on one single remaining worker... would it be insane to use a non-atomic `int` here?

💬 pinheadmz commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2466909255)
c219b93c3b043de202bdf3c65b433fd17af2da89

nit: don't bother with the year any more... (applies to all new files)

💬 pinheadmz commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469957844)
c219b93c3b043de202bdf3c65b433fd17af2da89

[As mentioned before](https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228839630), not a blocker but i feel like sleeps like this are test-flakiness waiting to happen ...

💬 pinheadmz commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469965455)
c219b93c3b043de202bdf3c65b433fd17af2da89

belt-and-suspenders, could
`BOOST_CHECK_EQUAL(threadPool.WorkQueueSize(), 0);`
I just like the symmetry in a test since you assert the value is `20` before taking action.

💬 ajtowns commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763)
> > What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
>
> Ignoring the initial self-announcement of the peer if it get mixed up with other addrs would feel strange.

Yeah, you're right. Though as glozow points out, it's also strange to have our sender/receiver side logic randomly mismatch depending on network behaviour (with this PR making the random mismatch much less frequent). Would it make sense to change the l
...

💬 ajtowns commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462304084)
> If they weren't shuffled on receipt, I think the self-announcement would always be processed.

I think shuffling helps avoid an attacker being able to (a) prevent honest nodes' addresses from being relayed (by filling up the buffer, so that later honest announcements get dropped) and (b) (easily?) detect how many addresses are being rate limited. So I don't think removing the shuffle would be a good idea.

💬 gmaxwell commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3462316950)
Luke-jr's seed returns knots 29.2 nodes, and yet he claims knots 29.2 is based on the same code it currently won't return. https://x.com/LukeDashjr/status/1977355254510256135 even though the issue is now known to luke-jr the behavior hasn't been correct.

I looked at my logs and see examples of discussions of luke-jr's seed it returning just released versions. The only exclusions I see discussed are e.g. old versions that didn't support node witness or have other known issues that make them un
...

⚠️ maflcko opened an issue: "ci: Lint task caching does not work?"
(https://github.com/bitcoin/bitcoin/issues/33735)

_Originally posted by @maflcko in [#33640](https://github.com/bitcoin/bitcoin/issues/33640#issuecomment-3458556346)_

I presume the same issue exists for the lint task. Though, the caching there does not seem to work at all. So it could make sense to make the caching work there, and then also add a retry loop there.


Otherwise, there could be Ubuntu APT timeouts like https://github.com/bitcoin/bitcoin/actions/runs/18861389218/job/53820273763?pr=33247#step:4:1053

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473904846)
> [c219b93](https://github.com/bitcoin/bitcoin/commit/c219b93c3b043de202bdf3c65b433fd17af2da89)
>
> I was wondering if there's any way to assert that the unblocked tasks are all executing on one single remaining worker... would it be insane to use a non-atomic `int` here?

hmm, even if the non-atomic int works, that doesn't really guarantee that the increment was done in the same thread.
If we want to be 100% correct, we should store the ids of the threads that processed the tasks on a syn
...

💬 ajtowns commented on issue "Decouple datacarrier size and count limits (Draft PR)":
(https://github.com/bitcoin/bitcoin/issues/33595#issuecomment-3462376140)
I think this PR would be better titled as "Make policy acceptance of multiple datacarrier outputs a configurable option", with the idea being to either introduce a boolean ("-multipledatacarrier=1" by default, allowing multiple datacarrier outputs; "-multipledatacarrier=0" as an option, reverting to the 29.x and earlier default of allowing one output per transaction, and "-datacarrier=0" to not allow any such outputs) or a count ("-datacarriercount=10000" by default, being effectively unlimited,
...

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473946974)
> [c219b93](https://github.com/bitcoin/bitcoin/commit/c219b93c3b043de202bdf3c65b433fd17af2da89)
>
> [As mentioned before](https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2228839630), not a blocker but i feel like sleeps like this are test-flakiness waiting to happen ...

This is one of those "wait and see if something happens" scenarios (if any task gets processed). We expect no activity here since all workers are busy.
I'm not sure there is another way of doing this, but if it f
...

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473982880)
Sounds good. Done as suggested. Thanks

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2473984516)
Sure. Done as suggested.