Bitcoin Core Github
44 subscribers
121K links
Download Telegram
πŸ‘‹ maflcko's pull request is ready for review: "ci: Call docker exec from Python script to fix word splitting"
(https://github.com/bitcoin/bitcoin/pull/33732)
πŸ’¬ Crypt-iQ commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461908503)
> The only downside of it would be not attracting macOS people to fuzzing development (?)

I was also thinking about this, and it could turn away some people from writing fuzz tests. Maybe if the docs mentioned a specific working version as suggested by @maflcko above, but then somebody would need to test these versions. I've had issues where even updating to a new macOS version isn't possible without some hacks.
πŸ’¬ fanquake commented on pull request "ci: run native fuzz with MSAN job":
(https://github.com/bitcoin/bitcoin/pull/33626#issuecomment-3461989252)
`-md` runtime was 43 minutes with no caches (libccxx/depends/cacche).
πŸ’¬ furszy commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3461995390)
It’s definitely painful to run it on Mac. I'm not against officially deprecating it if that is the general consensus but it’s quite useful to be able to troubleshoot individual crashes/timeouts locally. It would be handy to have pinned versions with specific docs on how to make it work.
πŸ’¬ maflcko commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462061936)
What is the configure summary, before you start to compile? (The output of `cmake -B build -DENABLE_IPC=OFF -DBUILD_GUI=OFF -DWITH_QRENCODE=OFF`)

What is your XCode version?

You are certainly not using the brew installed boost lib.

What other boost libs are on your system?
πŸ’¬ maflcko commented on pull request "random: scope environ extern to macOS, BSDs and Illumos":
(https://github.com/bitcoin/bitcoin/pull/33714#discussion_r2472906261)
nit: I think the comment can be kept as-is, or just refer to "Necessary on the above platforms". Otherwise, it is just one more line to maintain.

Also, I wonder if this can just be checked at configure-time, but no strong opinion.
πŸ’¬ kanzure 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-3462103592)
I also have a question as to whether user agent version filtering was previously discussed as a matter of DNS seed policy. If it was, then perhaps someone would be kind enough to give a link to that discussion. This however seems to me like a smaller issue compared to the other considerations that are necessary for resolving this ticket.

> We also learned of an investigation following the hack and are unsure whether Luke has shared access to the server with investigators. It would be helpful if
...
πŸ’¬ stringintech commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2473721206)
I thought if I want to treat it as boolean, it might be better to introduce it as a flag with the dash prefix. Would it make more sense to you if I drop the dash and treat it as a chain name instead (where only regtest and mainnet are supported, and if absent, defaults to mainnet)?
πŸ’¬ fanquake commented on pull request "random: scope environ extern to macOS, BSDs and Illumos":
(https://github.com/bitcoin/bitcoin/pull/33714#discussion_r2473728115)
Re-modified the comment. Not sure there is too much value in moving this to configure.
πŸ’¬ stringintech commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2473728951)
Yes I think it is better to have validation on this.
πŸ€” 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 ...