💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1389521297)
Ok, lets use an example (from the newly added test):
`vToFetch = [2 ... 129]`
`state->pindexBestKnownBlock->nHeight = 301`
So the peer's height is `301`. We can assume they can provide blocks `[13 ... 301]`.
At the first iteration of the loop `pindex->nHeight` is 2 (`vToFetch[0]`).
The code ends up with `return;` because `301 - 2 > 286` and we never request any blocks from that peer.
Shouldn't we request blocks `[13 ... 301]` instead?
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1389521297)
Ok, lets use an example (from the newly added test):
`vToFetch = [2 ... 129]`
`state->pindexBestKnownBlock->nHeight = 301`
So the peer's height is `301`. We can assume they can provide blocks `[13 ... 301]`.
At the first iteration of the loop `pindex->nHeight` is 2 (`vToFetch[0]`).
The code ends up with `return;` because `301 - 2 > 286` and we never request any blocks from that peer.
Shouldn't we request blocks `[13 ... 301]` instead?
💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1389526651)
The test would fail if run standalone without the preceding tests. The failure is on line 78: `self.sync_blocks([miner, pruned_node])`. That is because the test relies on the pruned node being connected to the miner which is indeed the case - some leftover from previous tests. This diff makes it succeed when run standalone:
```diff
--- i/test/functional/p2p_node_network_limited.py
+++ w/test/functional/p2p_node_network_limited.py
@@ -54,25 +54,24 @@ class NodeNetworkLimitedTest(BitcoinTest
...
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1389526651)
The test would fail if run standalone without the preceding tests. The failure is on line 78: `self.sync_blocks([miner, pruned_node])`. That is because the test relies on the pruned node being connected to the miner which is indeed the case - some leftover from previous tests. This diff makes it succeed when run standalone:
```diff
--- i/test/functional/p2p_node_network_limited.py
+++ w/test/functional/p2p_node_network_limited.py
@@ -54,25 +54,24 @@ class NodeNetworkLimitedTest(BitcoinTest
...
💬 L0laL33tz commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805920351)
Cool, Im working on a new follow-up excluding the leveldb from randomization @maflcko @theStack
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805920351)
Cool, Im working on a new follow-up excluding the leveldb from randomization @maflcko @theStack
👍 stickies-v approved a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1724824472)
ACK 3b73acb3ec678f5099aa0d77178a0b0d50670c2b
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1724824472)
ACK 3b73acb3ec678f5099aa0d77178a0b0d50670c2b
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389421589)
nit: can use `CTxMemPoolEntryRef`
<details>
<summary>git diff on 3b73acb3ec</summary>
```diff
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 85c2195b13..7c905ca4f4 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -176,4 +176,6 @@ public:
mutable Epoch::Marker m_epoch_marker; //!< epoch when last touched, useful for graph algorithms
};
+using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef;
+
#endif // BITC
...
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389421589)
nit: can use `CTxMemPoolEntryRef`
<details>
<summary>git diff on 3b73acb3ec</summary>
```diff
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 85c2195b13..7c905ca4f4 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -176,4 +176,6 @@ public:
mutable Epoch::Marker m_epoch_marker; //!< epoch when last touched, useful for graph algorithms
};
+using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef;
+
#endif // BITC
...
💬 maflcko commented on pull request "depends: remove `PYTHONPATH` from config.site":
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806001689)
When was this last used?
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806001689)
When was this last used?
💬 fanquake commented on pull request "depends: remove `PYTHONPATH` from config.site":
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806003917)
> When was this last used?
When we were building DMGs for macOS.
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806003917)
> When was this last used?
When we were building DMGs for macOS.
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389594932)
Will push this, seems easy enough to re-ACK.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389594932)
Will push this, seems easy enough to re-ACK.
💬 kashifs commented on pull request "BIP324 integration":
(https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1389597811)
@sipa, am I missing something or should this line read:
`node_1_info = self.nodes[1].getpeerinfo()`
(https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1389597811)
@sipa, am I missing something or should this line read:
`node_1_info = self.nodes[1].getpeerinfo()`
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1806011505)
Updated 3b73acb3ec678f5099aa0d77178a0b0d50670c2b -> 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b ([simplifyMemPoolInteractions_15](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_15) -> [simplifyMemPoolInteractions_16](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_15..simplifyMemPoolInteractions_16))
* Applied @stickies-v's [patch](https://github.com/bitc
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1806011505)
Updated 3b73acb3ec678f5099aa0d77178a0b0d50670c2b -> 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b ([simplifyMemPoolInteractions_15](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_15) -> [simplifyMemPoolInteractions_16](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_15..simplifyMemPoolInteractions_16))
* Applied @stickies-v's [patch](https://github.com/bitc
...
💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389606616)
If you put this in the global namespace, might as well remove the duplicate from the inner scope?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389606616)
If you put this in the global namespace, might as well remove the duplicate from the inner scope?
💬 sipa commented on pull request "BIP324 integration":
(https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1389608289)
This indeed looks like a bug.
(https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1389608289)
This indeed looks like a bug.
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389615076)
Can it be replaced without declaration order issues?
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389615076)
Can it be replaced without declaration order issues?
💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389641144)
I tried that at first but couldn't figure a more concise way given the `Parents` and `Children` typedefs.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389641144)
I tried that at first but couldn't figure a more concise way given the `Parents` and `Children` typedefs.
👍 stickies-v approved a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1725182406)
re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1725182406)
re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b
💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1806089364)
reACK 4dd94ca
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1806089364)
reACK 4dd94ca
📝 fanquake opened a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846)
Two changes to improve the libmultiprocess build on aarch64.
First, is to always install libmultiprocess into `lib/`. On some systems (my Fedora aarch64 box), libmultiprocess was being installed into `lib64/`, and then configure would fail to pick it up, because we only add `lib/` to pkgconfig/ldflags out of depends. Rather than adding lib64 to those, I opted for installing libmultiprocess into lib, with every other dependency we build.
Second, is to build with position independant code. T
...
(https://github.com/bitcoin/bitcoin/pull/28846)
Two changes to improve the libmultiprocess build on aarch64.
First, is to always install libmultiprocess into `lib/`. On some systems (my Fedora aarch64 box), libmultiprocess was being installed into `lib64/`, and then configure would fail to pick it up, because we only add `lib/` to pkgconfig/ldflags out of depends. Rather than adding lib64 to those, I opted for installing libmultiprocess into lib, with every other dependency we build.
Second, is to build with position independant code. T
...
💬 hebasto commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1806115806)
> Another thing to note, is that the code being patched here, has been deleted entirely upstream: [qt/qtbase@329db8b](https://github.com/qt/qtbase/commit/329db8b64f17c8ef013c586cea1f1c5b49c4a4b7).
>
> > macOS: Remove fallback defines for MAC_OS_X_VERSION_MIN_REQUIRED
> > Availability.h from the platform SDK should take care of this these days.
>
> So this points to there still being an underlying issue...
The fact is that the `__MAC_OS_X_VERSION_MIN_REQUIRED` is [_not_](https://github.
...
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1806115806)
> Another thing to note, is that the code being patched here, has been deleted entirely upstream: [qt/qtbase@329db8b](https://github.com/qt/qtbase/commit/329db8b64f17c8ef013c586cea1f1c5b49c4a4b7).
>
> > macOS: Remove fallback defines for MAC_OS_X_VERSION_MIN_REQUIRED
> > Availability.h from the platform SDK should take care of this these days.
>
> So this points to there still being an underlying issue...
The fact is that the `__MAC_OS_X_VERSION_MIN_REQUIRED` is [_not_](https://github.
...
💬 hebasto commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1806128909)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1806128909)
Concept ACK.
💬 fanquake commented on pull request "build: Patch Qt to handle minimum macOS version properly":
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1806142446)
> At least, when using the current Clang version in depends.
Right, but why is it suddenly defined when using a newer compiler? Can you point to where in the LLVM/Clang source code this starts happening in versions 16+? It just irks me that updating to a new compiler "magically" fixes things, without explanation.
I think we will likely take this patch as a temporary workaround, just to unblock C++20, but we should probably add some more context to the patch itself. I also dislike having to
...
(https://github.com/bitcoin/bitcoin/pull/28775#issuecomment-1806142446)
> At least, when using the current Clang version in depends.
Right, but why is it suddenly defined when using a newer compiler? Can you point to where in the LLVM/Clang source code this starts happening in versions 16+? It just irks me that updating to a new compiler "magically" fixes things, without explanation.
I think we will likely take this patch as a temporary workaround, just to unblock C++20, but we should probably add some more context to the patch itself. I also dislike having to
...