💬 TheCharlatan commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173594457)
Doesn't this leave us with two pointers, one in `m_banman` and the other in `m_opts.banman` that is never used?
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173594457)
Doesn't this leave us with two pointers, one in `m_banman` and the other in `m_opts.banman` that is never used?
💬 TheCharlatan commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173600064)
Could reading from the args be moved to their own `ApplyArgsManOptions` function like in various `node/*_args.cpp` files? Then the test code wouldn't have to repeat the logic here.
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173600064)
Could reading from the args be moved to their own `ApplyArgsManOptions` function like in various `node/*_args.cpp` files? Then the test code wouldn't have to repeat the logic here.
🚀 fanquake merged a pull request: "test: Remove unused sanitizer suppressions"
(https://github.com/bitcoin/bitcoin/pull/27498)
(https://github.com/bitcoin/bitcoin/pull/27498)
💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173615788)
I thought it makes sense because the banman is optional but happy to move it out, no strong opinion on this. Let me know what you prefer.
Will leave this unresolved for a bit to let others chime in as well.
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173615788)
I thought it makes sense because the banman is optional but happy to move it out, no strong opinion on this. Let me know what you prefer.
Will leave this unresolved for a bit to let others chime in as well.
💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173616551)
Yea, could get rid of m_banman or not have banman in the options in the first place...
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1173616551)
Yea, could get rid of m_banman or not have banman in the options in the first place...
💬 TheCharlatan commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#issuecomment-1517635032)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27499#issuecomment-1517635032)
Concept ACK
💬 fanquake commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517638047)
Should also bump the lint DOckerfile to `python:3.8-buster`?
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517638047)
Should also bump the lint DOckerfile to `python:3.8-buster`?
📝 fanquake opened a pull request: "build: use latest config.{guess,sub} in depends"
(https://github.com/bitcoin/bitcoin/pull/27508)
Been a few years since we last updated these.
Also related to https://github.com/bitcoin/bitcoin/pull/26422#issuecomment-1421178967.
(https://github.com/bitcoin/bitcoin/pull/27508)
Been a few years since we last updated these.
Also related to https://github.com/bitcoin/bitcoin/pull/26422#issuecomment-1421178967.
🚀 fanquake merged a pull request: "fuzz: re-enable prioritisetransaction & analyzepsbt RPC"
(https://github.com/bitcoin/bitcoin/pull/27464)
(https://github.com/bitcoin/bitcoin/pull/27464)
👋 fanquake's pull request is ready for review: "[23.x] Additional backports for 23.x"
(https://github.com/bitcoin/bitcoin/pull/27475)
(https://github.com/bitcoin/bitcoin/pull/27475)
💬 hebasto commented on pull request "build: use latest config.{guess,sub} in depends":
(https://github.com/bitcoin/bitcoin/pull/27508#issuecomment-1517649284)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27508#issuecomment-1517649284)
Concept ACK.
💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1173636442)
@fkorotkov can you offer any insight here? Should we be able to use non-LTS Ubuntu images?
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1173636442)
@fkorotkov can you offer any insight here? Should we be able to use non-LTS Ubuntu images?
💬 pinheadmz commented on pull request "net: use interruptible async getaddrinfo wrapper from libevent for DNS":
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1517682872)
@dergoegge I think the CI failures are memory leaks from my code, I'm going to fix that. But I hear your point about the arcane library. Any suggestions?
(https://github.com/bitcoin/bitcoin/pull/27505#issuecomment-1517682872)
@dergoegge I think the CI failures are memory leaks from my code, I'm going to fix that. But I hear your point about the arcane library. Any suggestions?
💬 hebasto commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1517728829)
Rebased d462e3da7fc6be75269e88928fd80fc98c405474 -> 52129e335cbc68ac5d863f283f7d1a328ce79581 ([pr26762.07](https://github.com/hebasto/bitcoin/commits/pr26762.07) -> [pr26762.08](https://github.com/hebasto/bitcoin/commits/pr26762.08)) due to the conflict with #27419.
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1517728829)
Rebased d462e3da7fc6be75269e88928fd80fc98c405474 -> 52129e335cbc68ac5d863f283f7d1a328ce79581 ([pr26762.07](https://github.com/hebasto/bitcoin/commits/pr26762.07) -> [pr26762.08](https://github.com/hebasto/bitcoin/commits/pr26762.08)) due to the conflict with #27419.
💬 MarcoFalke commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517739626)
> Should also bump the lint Dockerfile to python:3.8-buster?
buster is EOL and unmaintained, which is one of the reasons I created this pull, as can be seen in the pull request description. I don't really understand why the lint Dockerfile isn't simply using the exact same distro and setup like the lint CI. Using something else is just going to make it less reproducible and harder to maintain. Though, those changes should probably be made in a separate follow-up pull.
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517739626)
> Should also bump the lint Dockerfile to python:3.8-buster?
buster is EOL and unmaintained, which is one of the reasons I created this pull, as can be seen in the pull request description. I don't really understand why the lint Dockerfile isn't simply using the exact same distro and setup like the lint CI. Using something else is just going to make it less reproducible and harder to maintain. Though, those changes should probably be made in a separate follow-up pull.
💬 fanquake commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517765508)
> buster is EOL and unmaintained,
In this context, I don't think that makes any difference, as the container is just an interpreter, so as long as it's downloadable, it should work (no installing packages etc).
> I don't really understand why the lint Dockerfile isn't simply using the exact same distro and setup like the lint CI.
No idea. Maybe ask @jamesob.
I was mostly pointing it out because it seemed odd to not just bump it as well. Not a blocker if someone else is going to follo
...
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517765508)
> buster is EOL and unmaintained,
In this context, I don't think that makes any difference, as the container is just an interpreter, so as long as it's downloadable, it should work (no installing packages etc).
> I don't really understand why the lint Dockerfile isn't simply using the exact same distro and setup like the lint CI.
No idea. Maybe ask @jamesob.
I was mostly pointing it out because it seemed odd to not just bump it as well. Not a blocker if someone else is going to follo
...
💬 fanquake commented on pull request "build: use latest config.{guess,sub} in depends":
(https://github.com/bitcoin/bitcoin/pull/27508#issuecomment-1517769106)
Guix Build:
```bash
fef9152d593aa1cb3243f37b8c069ef863f4e270962e17918cfa83e18c9eac3a guix-build-4a3f1db4ea5f/output/aarch64-linux-gnu/SHA256SUMS.part
3fa8236cea2ba1a08d0caf677de80f798f2594cb208f10e5dfe0a9a71de7978d guix-build-4a3f1db4ea5f/output/aarch64-linux-gnu/bitcoin-4a3f1db4ea5f-aarch64-linux-gnu-debug.tar.gz
36409f4fa6acfb577d6e6a43c2fda2d81f1fb6fb4e5d90cbaa65a2dbb26d1c23 guix-build-4a3f1db4ea5f/output/aarch64-linux-gnu/bitcoin-4a3f1db4ea5f-aarch64-linux-gnu.tar.gz
02a62c4cc2ecb63b
...
(https://github.com/bitcoin/bitcoin/pull/27508#issuecomment-1517769106)
Guix Build:
```bash
fef9152d593aa1cb3243f37b8c069ef863f4e270962e17918cfa83e18c9eac3a guix-build-4a3f1db4ea5f/output/aarch64-linux-gnu/SHA256SUMS.part
3fa8236cea2ba1a08d0caf677de80f798f2594cb208f10e5dfe0a9a71de7978d guix-build-4a3f1db4ea5f/output/aarch64-linux-gnu/bitcoin-4a3f1db4ea5f-aarch64-linux-gnu-debug.tar.gz
36409f4fa6acfb577d6e6a43c2fda2d81f1fb6fb4e5d90cbaa65a2dbb26d1c23 guix-build-4a3f1db4ea5f/output/aarch64-linux-gnu/bitcoin-4a3f1db4ea5f-aarch64-linux-gnu.tar.gz
02a62c4cc2ecb63b
...
👍 fanquake approved a pull request: "Bump python minimum version to 3.8"
(https://github.com/bitcoin/bitcoin/pull/27483#pullrequestreview-1395687975)
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3
(https://github.com/bitcoin/bitcoin/pull/27483#pullrequestreview-1395687975)
ACK fac395e5eb2cd3210ba6345f777a586a9bec84e3
💬 MarcoFalke commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517774746)
> it should work (no apt installing packages etc).
It calls `RUN /install.sh`, which installs packages
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517774746)
> it should work (no apt installing packages etc).
It calls `RUN /install.sh`, which installs packages
💬 fanquake commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517780543)
> It calls RUN /install.sh, which installs packages
Right. Although it doesn't actually need to do that, as all the packages we needs are already available in the base python3.8-buster image.
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517780543)
> It calls RUN /install.sh, which installs packages
Right. Although it doesn't actually need to do that, as all the packages we needs are already available in the base python3.8-buster image.