💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-1561833344)
> This change ignores non-main network data.
Ah I see, because I'm checking for `Bitcoin\chainstate` specifically but if a user only ever used signet, there would be a nested `signet` directory in there too. Is there anything else I can check for that always gets written to that root? Or maybe just check if `Bitcoin\` is empty at all?
> I'm OK with that. But it seems worth mentioning in the docs.
Sure but I feel like that's not super clean either.
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-1561833344)
> This change ignores non-main network data.
Ah I see, because I'm checking for `Bitcoin\chainstate` specifically but if a user only ever used signet, there would be a nested `signet` directory in there too. Is there anything else I can check for that always gets written to that root? Or maybe just check if `Bitcoin\` is empty at all?
> I'm OK with that. But it seems worth mentioning in the docs.
Sure but I feel like that's not super clean either.
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1204686737)
done
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1204686737)
done
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1204687270)
Sounds good, I am using this now.
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1204687270)
Sounds good, I am using this now.
👋 fjahr's pull request is ready for review: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581)
(https://github.com/bitcoin/bitcoin/pull/27581)
💬 amitiuttarwar commented on pull request "fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`":
(https://github.com/bitcoin/bitcoin/pull/27549#issuecomment-1561847276)
it looks like this PR provides the same coverage as 8f91e5898b3571e0802f2197981c54dda9ee71fa in #27213. I'll remove the commit there so 27213 can focus on discussing the changes to node behavior
(https://github.com/bitcoin/bitcoin/pull/27549#issuecomment-1561847276)
it looks like this PR provides the same coverage as 8f91e5898b3571e0802f2197981c54dda9ee71fa in #27213. I'll remove the commit there so 27213 can focus on discussing the changes to node behavior
💬 amitiuttarwar commented on pull request "fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()`":
(https://github.com/bitcoin/bitcoin/pull/27549#issuecomment-1561848392)
for the record, ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838 - only small changes from the version (previously) proposed in 27213
(https://github.com/bitcoin/bitcoin/pull/27549#issuecomment-1561848392)
for the record, ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838 - only small changes from the version (previously) proposed in 27213
📝 sdaftuar opened a pull request: "Draft: rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746)
This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation.
This PR also fixes
...
(https://github.com/bitcoin/bitcoin/pull/27746)
This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation.
This PR also fixes
...
💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1561849558)
I think this is ready for review. I have included the feedback so far and I think the current extent of the checks and stats is good enough as a first step. I am currently still going through all the possible cases we could see an ASMap file be harmful to a node and this may lead to some additional checks and stats being logged in the future but I need some more time for this and it can be done as a follow-up.
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1561849558)
I think this is ready for review. I have included the feedback so far and I think the current extent of the checks and stats is good enough as a first step. I am currently still going through all the possible cases we could see an ASMap file be harmful to a node and this may lead to some additional checks and stats being logged in the future but I need some more time for this and it can be done as a follow-up.
💬 ArmchairCryptologist commented on issue "Frequent "Timeout downloading block" with 24.1":
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561850433)
> I agree, with `addr=x.x.x.x` being conditional on `-logips`. That could make sense in some more spots as well, I could open a PR to suggest it unless you want to.
By all means, please do. At least with this it will be trivial to just block any peers that repeatedly cause the node to stall.
(https://github.com/bitcoin/bitcoin/issues/27705#issuecomment-1561850433)
> I agree, with `addr=x.x.x.x` being conditional on `-logips`. That could make sense in some more spots as well, I could open a PR to suggest it unless you want to.
By all means, please do. At least with this it will be trivial to just block any peers that repeatedly cause the node to stall.
💬 jamesob commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1561858383)
Great! Thanks for this. I'm active in these neighborhoods today as well, since adding some `-stopatheight`/restart logic to the functional tests in #27596 has turned up some issues with `CheckBlockIndex()`, which are maybe related to some of the changes here. So I'll be looking at this shortly.
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1561858383)
Great! Thanks for this. I'm active in these neighborhoods today as well, since adding some `-stopatheight`/restart logic to the functional tests in #27596 has turned up some issues with `CheckBlockIndex()`, which are maybe related to some of the changes here. So I'll be looking at this shortly.
💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1561870144)
I'll give this another look next week.
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1561870144)
I'll give this another look next week.
💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1561872278)
this PR is ready for review!
did a couple of clean-up steps:
* removed the fuzz commit since coverage is now handled in #27549
* rebased on master
* updated the OP to reflect current status
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1561872278)
this PR is ready for review!
did a couple of clean-up steps:
* removed the fuzz commit since coverage is now handled in #27549
* rebased on master
* updated the OP to reflect current status
👋 amitiuttarwar's pull request is ready for review: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213)
(https://github.com/bitcoin/bitcoin/pull/27213)
👍 pinheadmz approved a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1442687736)
re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
Confirmed nits addressed since last ACK, minimal diff. All tests pass locally for me, CI failures are unrelated to code changes.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRucOIACgkQ5+KYS2KJ
yTpqZg//YoQc3ivctelv3QQLCGcGruDO9twOQMm8TRx+8mZRtIV2UqPom2pwLjXM
BnZ
...
(https://github.com/bitcoin/bitcoin/pull/27302#pullrequestreview-1442687736)
re-ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
Confirmed nits addressed since last ACK, minimal diff. All tests pass locally for me, CI failures are unrelated to code changes.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK eefe56967b4eb4b5144325cde4f40fc1cbde3e65
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRucOIACgkQ5+KYS2KJ
yTpqZg//YoQc3ivctelv3QQLCGcGruDO9twOQMm8TRx+8mZRtIV2UqPom2pwLjXM
BnZ
...
💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204729225)
s/121/112/ ?
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204729225)
s/121/112/ ?
💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1561897120)
Added a custom forked guix repo containing the patch we need for llvm's build.
I *believe* I've fixed authorization and it should just work(tm).
My local guix build works now, but I've reworked things so many times it would be helpful to get a confirmation.
Going to mark as ready for review since guix builds might actually work now.
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1561897120)
Added a custom forked guix repo containing the patch we need for llvm's build.
I *believe* I've fixed authorization and it should just work(tm).
My local guix build works now, but I've reworked things so many times it would be helpful to get a confirmation.
Going to mark as ready for review since guix builds might actually work now.
👋 theuni's pull request is ready for review: "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM"
(https://github.com/bitcoin/bitcoin/pull/27676)
(https://github.com/bitcoin/bitcoin/pull/27676)
💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204739973)
taproot coinbase outputs wen
LGTM
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204739973)
taproot coinbase outputs wen
LGTM
👍 instagibbs approved a pull request: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711#pullrequestreview-1442724105)
ACK https://github.com/bitcoin/bitcoin/pull/26711/commits/b234f993da3caa16f7716948fbcd16ab1a500180
modulo magic numbers in tests (sorry). Ran the fuzzer for a while again, no issues this time.
(https://github.com/bitcoin/bitcoin/pull/26711#pullrequestreview-1442724105)
ACK https://github.com/bitcoin/bitcoin/pull/26711/commits/b234f993da3caa16f7716948fbcd16ab1a500180
modulo magic numbers in tests (sorry). Ran the fuzzer for a while again, no issues this time.
💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204742880)
diamond keeps going "out of sync", maybe just have variable names that people can find?
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1204742880)
diamond keeps going "out of sync", maybe just have variable names that people can find?