💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2426960596)
@fanquake thanks, I'll look into that along with a rebase - might be after TABConf.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2426960596)
@fanquake thanks, I'll look into that along with a rebase - might be after TABConf.
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2426964142)
cc @theuni any takes?
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2426964142)
cc @theuni any takes?
🤔 marcofleon reviewed a pull request: "test: Fuzz the human-readable part of bech32 as well"
(https://github.com/bitcoin/bitcoin/pull/30623#pullrequestreview-2382423311)
Code review ACK 9b7023d31a3ec95f66b45f0ecb47e79762d74854. The separation into two targets and the new `GenerateRandomHRP` seem fine to me.
Decode [coverage](https://marcofleon.github.io/coverage/bech32decode/)
Roundtrip [coverage](https://marcofleon.github.io/coverage/bech32roundtrip/)
(https://github.com/bitcoin/bitcoin/pull/30623#pullrequestreview-2382423311)
Code review ACK 9b7023d31a3ec95f66b45f0ecb47e79762d74854. The separation into two targets and the new `GenerateRandomHRP` seem fine to me.
Decode [coverage](https://marcofleon.github.io/coverage/bech32decode/)
Roundtrip [coverage](https://marcofleon.github.io/coverage/bech32roundtrip/)
💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2426976364)
> This includes the ability to create a new block in between validating a new block and updating the mempool for a full CNB run.
The interface isn't final so that could be added. But aren't we holding `cs_main` throughout the process? cc @TheCharlatan, @ryanofsky
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2426976364)
> This includes the ability to create a new block in between validating a new block and updating the mempool for a full CNB run.
The interface isn't final so that could be added. But aren't we holding `cs_main` throughout the process? cc @TheCharlatan, @ryanofsky
🤔 jonatack reviewed a pull request: "rpc: net: follow-ups for #30062"
(https://github.com/bitcoin/bitcoin/pull/30183#pullrequestreview-2382439587)
LGTM but the commit could be more accurately renamed to "Improve mapped AS documentation" and perhaps touch up this doc in the same commit:
```diff
--- a/src/netgroup.h
+++ b/src/netgroup.h
@@ -35,9 +35,9 @@ public:
std::vector<unsigned char> GetGroup(const CNetAddr& address) const;
/**
- * Get the autonomous system on the BGP path to address.
+ * Get the autonomous system number at the end of the BGP path to the IP address.
*
- * The ip->AS mapping
...
(https://github.com/bitcoin/bitcoin/pull/30183#pullrequestreview-2382439587)
LGTM but the commit could be more accurately renamed to "Improve mapped AS documentation" and perhaps touch up this doc in the same commit:
```diff
--- a/src/netgroup.h
+++ b/src/netgroup.h
@@ -35,9 +35,9 @@ public:
std::vector<unsigned char> GetGroup(const CNetAddr& address) const;
/**
- * Get the autonomous system on the BGP path to address.
+ * Get the autonomous system number at the end of the BGP path to the IP address.
*
- * The ip->AS mapping
...
💬 Sjors commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1809026225)
That might make sense, though I'm not sure how much these round trips matter when running on the same machine.
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1809026225)
That might make sense, though I'm not sure how much these round trips matter when running on the same machine.
💬 Sjors commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1809027825)
I don't think we should be trying to make the IPC interface DoS resistant. We don't do that for RPC either.
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1809027825)
I don't think we should be trying to make the IPC interface DoS resistant. We don't do that for RPC either.
💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2426998599)
Lightly tested native builds on Debian 12.7 and Ubuntu 24.04 using the qt6 packages described in build-unix.md. Found no regression compared to Qt5. Will test guix builds next.
fwiw: this can be trivially rebased by dropping the MacOS 12 commit, all the conflicts are in that.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2426998599)
Lightly tested native builds on Debian 12.7 and Ubuntu 24.04 using the qt6 packages described in build-unix.md. Found no regression compared to Qt5. Will test guix builds next.
fwiw: this can be trivially rebased by dropping the MacOS 12 commit, all the conflicts are in that.
💬 Sjors commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2427009621)
> Do you think the lack of desire for review (compared to reviewing the capnproto protocol instead) comes from the encryption, the framing logic, the connman management, etc?
The "instead" misses the mark: reviewing capnproto was already necessary to achieve all the other use cases of multiprocess. The mining interface just provided an easier opportunity to make progress there.
Trying to redesign the interface in a way that makes it DoS-safe brings back the need for a lot of the review tha
...
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2427009621)
> Do you think the lack of desire for review (compared to reviewing the capnproto protocol instead) comes from the encryption, the framing logic, the connman management, etc?
The "instead" misses the mark: reviewing capnproto was already necessary to achieve all the other use cases of multiprocess. The mining interface just provided an easier opportunity to make progress there.
Trying to redesign the interface in a way that makes it DoS-safe brings back the need for a lot of the review tha
...
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1809048162)
done, previously this would have allowed non-dust-spending children and invalidate the invariant checks, now it shouldn't matter
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1809048162)
done, previously this would have allowed non-dust-spending children and invalidate the invariant checks, now it shouldn't matter
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1809048220)
done
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1809048220)
done
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1809050952)
> we don't have CTxMemPoolEntry objects to work on yet
I don't think that's true? `PreChecks` has built a mempool entry for each package tx. We could track the exact outpoint that is dust in the mempool entry. I'll give that a shot.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1809050952)
> we don't have CTxMemPoolEntry objects to work on yet
I don't think that's true? `PreChecks` has built a mempool entry for each package tx. We could track the exact outpoint that is dust in the mempool entry. I'll give that a shot.
✅ Sjors closed an issue: "macOS 15.0.1 can't build Qt (Intel, without Xcode)"
(https://github.com/bitcoin/bitcoin/issues/31054)
(https://github.com/bitcoin/bitcoin/issues/31054)
💬 Sjors commented on issue "macOS 15.0.1 can't build Qt (Intel, without Xcode)":
(https://github.com/bitcoin/bitcoin/issues/31054#issuecomment-2427030422)
Going to close this for now, since you can't reproduce on Intel.
(https://github.com/bitcoin/bitcoin/issues/31054#issuecomment-2427030422)
Going to close this for now, since you can't reproduce on Intel.
💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2427035858)
Seems like I need to look into and probably fix at least https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791784114
Will look into the other comments too.
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2427035858)
Seems like I need to look into and probably fix at least https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791784114
Will look into the other comments too.
💬 Christewart commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2427058842)
> I'm a bit confused to the behavior i'm observing on this PR. Not necessarily anything to do with the test, but how orphans are handled.
>
> I tried adding another part to the test case to check we can clear the orphanage by propagating parent transactions: [Christewart@04815c4](https://github.com/Christewart/bitcoin/commit/04815c463d331f674f67fdcb3b8da601484d7a33)
>
> It appears that to _totally_ clear the orphanage, we need to propagate the parent transaction for the last child transact
...
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2427058842)
> I'm a bit confused to the behavior i'm observing on this PR. Not necessarily anything to do with the test, but how orphans are handled.
>
> I tried adding another part to the test case to check we can clear the orphanage by propagating parent transactions: [Christewart@04815c4](https://github.com/Christewart/bitcoin/commit/04815c463d331f674f67fdcb3b8da601484d7a33)
>
> It appears that to _totally_ clear the orphanage, we need to propagate the parent transaction for the last child transact
...
💬 cdecker commented on pull request "chainparams: Re-add seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2427073328)
Oh, that's a good data point. Seems the result is larger than your ISP is willing to forward. Would you like me to return fewer results?
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2427073328)
Oh, that's a good data point. Seems the result is larger than your ISP is willing to forward. Would you like me to return fewer results?
💬 Sjors commented on issue "macOS 13.7 depends build can't find qt":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2427112271)
Tried https://github.com/bitcoin/bitcoin/commit/aba02f0c06fdcf28fe76de4cf00dc73c3aee6208 with `make NO_WALLET=1 NO_UPNP=1 NO_ZMQ=1 NO_BDB=1` so save some build time:
```
...
copying packages: boost libevent qt qrencode
to: /Volumes/SSD/Dev/bitcoin/depends/x86_64-apple-darwin22.6.0
To build Bitcoin Core with these packages, pass '--toolchain /Volumes/SSD/Dev/bitcoin/depends/x86_64-apple-darwin22.6.0/toolchain.cmake' to the first CMake invocation.
sjors@Sjorss-iMac depends % cd ..
sjors@
...
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2427112271)
Tried https://github.com/bitcoin/bitcoin/commit/aba02f0c06fdcf28fe76de4cf00dc73c3aee6208 with `make NO_WALLET=1 NO_UPNP=1 NO_ZMQ=1 NO_BDB=1` so save some build time:
```
...
copying packages: boost libevent qt qrencode
to: /Volumes/SSD/Dev/bitcoin/depends/x86_64-apple-darwin22.6.0
To build Bitcoin Core with these packages, pass '--toolchain /Volumes/SSD/Dev/bitcoin/depends/x86_64-apple-darwin22.6.0/toolchain.cmake' to the first CMake invocation.
sjors@Sjorss-iMac depends % cd ..
sjors@
...
💬 stickies-v commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1809122657)
> You _must_ include unconfirmed parents, if any exist.
Thanks, you're right. I was confused by the `IsChildWithParents` documentation stating:
> ...not all parents need to be present...
But that is of course just an incomplete, context-free check. The `MemPoolAccept::AcceptPackage` documentation and implementation indeed require all unconfirmed parents to be included. Your [suggestion](https://github.com/bitcoin/bitcoin/compare/72872c7073e99da0828e788b25520dcecf721c1b..92ed1bd586f7048f
...
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1809122657)
> You _must_ include unconfirmed parents, if any exist.
Thanks, you're right. I was confused by the `IsChildWithParents` documentation stating:
> ...not all parents need to be present...
But that is of course just an incomplete, context-free check. The `MemPoolAccept::AcceptPackage` documentation and implementation indeed require all unconfirmed parents to be included. Your [suggestion](https://github.com/bitcoin/bitcoin/compare/72872c7073e99da0828e788b25520dcecf721c1b..92ed1bd586f7048f
...
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1809129112)
can do when touched again
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1809129112)
can do when touched again
🤔 hebasto reviewed a pull request: "guix: Enable CET for `glibc` package"
(https://github.com/bitcoin/bitcoin/pull/31121#pullrequestreview-2382645014)
My Guix build:
```
aarch64
f661515f39c1fef7b7ab7e1de3665ccf3efba9ffbcc6c4de396962e9c6b1a92e guix-build-4d3da08d1b9d/output/aarch64-linux-gnu/SHA256SUMS.part
f4889a72693e3249ac5d59b83c23ddb74f15d9e844c131ae41e17fd5daa15772 guix-build-4d3da08d1b9d/output/aarch64-linux-gnu/bitcoin-4d3da08d1b9d-aarch64-linux-gnu-debug.tar.gz
68dc8bb68f0682047357d292ee71e7b654e3aa1431f50a1ae9ef459f51318663 guix-build-4d3da08d1b9d/output/aarch64-linux-gnu/bitcoin-4d3da08d1b9d-aarch64-linux-gnu.tar.gz
9ad44c30
...
(https://github.com/bitcoin/bitcoin/pull/31121#pullrequestreview-2382645014)
My Guix build:
```
aarch64
f661515f39c1fef7b7ab7e1de3665ccf3efba9ffbcc6c4de396962e9c6b1a92e guix-build-4d3da08d1b9d/output/aarch64-linux-gnu/SHA256SUMS.part
f4889a72693e3249ac5d59b83c23ddb74f15d9e844c131ae41e17fd5daa15772 guix-build-4d3da08d1b9d/output/aarch64-linux-gnu/bitcoin-4d3da08d1b9d-aarch64-linux-gnu-debug.tar.gz
68dc8bb68f0682047357d292ee71e7b654e3aa1431f50a1ae9ef459f51318663 guix-build-4d3da08d1b9d/output/aarch64-linux-gnu/bitcoin-4d3da08d1b9d-aarch64-linux-gnu.tar.gz
9ad44c30
...