💬 maflcko commented on issue "Failing to fetch `cfheader` corresponding to block header in `headers` message":
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1878994053)
> I am calling syncwithvalidationinterfacequeue explicitly.
Are you sure, because the debug log you shared does not mention it?
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1878994053)
> I am calling syncwithvalidationinterfacequeue explicitly.
Are you sure, because the debug log you shared does not mention it?
👍 TheCharlatan approved a pull request: "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256"
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1806423145)
ACK 86712c3135786b305f27c44dffd0808be0ee7170
Guix builds (aarch64):
```
e00ac9d2dbf9d1b6a0255d7c6669ee3925696109cf4ca549dbf411da235f52bc guix-build-86712c313578/output/aarch64-linux-gnu/SHA256SUMS.part
871f7e66c454ac08d4e04f0d86d23e2b34a6c3a6a3b60aabcb9485c086cdad37 guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-86712c313578-aarch64-linux-gnu-debug.tar.gz
f0553ca6ebaaf5eb8d00196a681a20c21d4b81cb3ec907cce01ac6a446de02e7 guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-
...
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1806423145)
ACK 86712c3135786b305f27c44dffd0808be0ee7170
Guix builds (aarch64):
```
e00ac9d2dbf9d1b6a0255d7c6669ee3925696109cf4ca549dbf411da235f52bc guix-build-86712c313578/output/aarch64-linux-gnu/SHA256SUMS.part
871f7e66c454ac08d4e04f0d86d23e2b34a6c3a6a3b60aabcb9485c086cdad37 guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-86712c313578-aarch64-linux-gnu-debug.tar.gz
f0553ca6ebaaf5eb8d00196a681a20c21d4b81cb3ec907cce01ac6a446de02e7 guix-build-86712c313578/output/aarch64-linux-gnu/bitcoin-
...
💬 theuni commented on pull request "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256":
(https://github.com/bitcoin/bitcoin/pull/29180#discussion_r1443133564)
😳 Whoops!
Fixed, thanks :)
(https://github.com/bitcoin/bitcoin/pull/29180#discussion_r1443133564)
😳 Whoops!
Fixed, thanks :)
👍 TheCharlatan approved a pull request: "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256"
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1806424122)
Re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1806424122)
Re-ACK bbf218d06164b7247f5e9df5ba143383022fbf74
👍 hebasto approved a pull request: "crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256"
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1806425852)
re-aCK bbf218d06164b7247f5e9df5ba143383022fbf74
(https://github.com/bitcoin/bitcoin/pull/29180#pullrequestreview-1806425852)
re-aCK bbf218d06164b7247f5e9df5ba143383022fbf74
💬 Christewart commented on issue "Failing to fetch `cfheader` corresponding to block header in `headers` message":
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1879005820)
> > I am calling syncwithvalidationinterfacequeue explicitly.
>
> Are you sure, because the debug log you shared does not mention it?
I've modified the OP to add those logs and i've attached the full `debug.log`.
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1879005820)
> > I am calling syncwithvalidationinterfacequeue explicitly.
>
> Are you sure, because the debug log you shared does not mention it?
I've modified the OP to add those logs and i've attached the full `debug.log`.
⚠️ so7ow opened an issue: "v26 release not available via HomeBrew"
(https://github.com/bitcoin/bitcoin/issues/29190)
I'm just curious how that works. Is there some reason v26 still isn't available on HomeBrew?
https://formulae.brew.sh/formula/bitcoin
(https://github.com/bitcoin/bitcoin/issues/29190)
I'm just curious how that works. Is there some reason v26 still isn't available on HomeBrew?
https://formulae.brew.sh/formula/bitcoin
💬 dongcarl commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879008298)
Might still be used here: https://github.com/rust-bitcoin/rust-bitcoinconsensus
Ping @apoelstra @tcharding
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879008298)
Might still be used here: https://github.com/rust-bitcoin/rust-bitcoinconsensus
Ping @apoelstra @tcharding
💬 luke-jr commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879009564)
>History of this config option suggests datacarriersize is meant to limit the size of data in OP_RETURN outputs, so this statement is untrue.
It's meant to limit extra data in transactions. OP_RETURN was supposed to be the only tolerated way to do that. `datacarriersize` has no possible use if it's trivial to bypass. The "Ordisrespector" approach would take us back to that prior status quo.
>why not propose a new config option?
It would be confusingly redundant. But if that is Concept A
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879009564)
>History of this config option suggests datacarriersize is meant to limit the size of data in OP_RETURN outputs, so this statement is untrue.
It's meant to limit extra data in transactions. OP_RETURN was supposed to be the only tolerated way to do that. `datacarriersize` has no possible use if it's trivial to bypass. The "Ordisrespector" approach would take us back to that prior status quo.
>why not propose a new config option?
It would be confusingly redundant. But if that is Concept A
...
💬 fanquake commented on issue "v26 release not available via HomeBrew":
(https://github.com/bitcoin/bitcoin/issues/29190#issuecomment-1879011162)
See https://github.com/Homebrew/homebrew-core/pull/156745. That PR is currently blocked on test-failures in the brew (Linux) CI.
(https://github.com/bitcoin/bitcoin/issues/29190#issuecomment-1879011162)
See https://github.com/Homebrew/homebrew-core/pull/156745. That PR is currently blocked on test-failures in the brew (Linux) CI.
💬 maflcko commented on issue "Failing to fetch `cfheader` corresponding to block header in `headers` message":
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1879014159)
Ok, it is called, but you also have to wait for it to complete and return from the RPC, before continuing the test. Currently it looks like it is called, and processes the block events, but at the same time the P2P interface is asked for the filters.
It may be best to only use a single thread for the test logic.
(https://github.com/bitcoin/bitcoin/issues/27085#issuecomment-1879014159)
Ok, it is called, but you also have to wait for it to complete and return from the RPC, before continuing the test. Currently it looks like it is called, and processes the block events, but at the same time the P2P interface is asked for the filters.
It may be best to only use a single thread for the test logic.
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1443146961)
I will address it in the next push.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1443146961)
I will address it in the next push.
📝 aureleoules opened a pull request: "refactor(tidy): Use C++20 contains method"
(https://github.com/bitcoin/bitcoin/pull/29191)
C++20 introduced the `contains` method on containers to check if an element is in the container. It can be used instead of the `count` method now.
I believe it is easier to understand, as `contains` directly returns a `bool` indicating whether the element exists, while `count` returns the number of occurrences of the element, which then often needs to be compared against 0.
Also, it is slightly more efficient than count for this use-case.
This pull request introduces the clang-tidy check `r
...
(https://github.com/bitcoin/bitcoin/pull/29191)
C++20 introduced the `contains` method on containers to check if an element is in the container. It can be used instead of the `count` method now.
I believe it is easier to understand, as `contains` directly returns a `bool` indicating whether the element exists, while `count` returns the number of occurrences of the element, which then often needs to be compared against 0.
Also, it is slightly more efficient than count for this use-case.
This pull request introduces the clang-tidy check `r
...
💬 so7ow commented on issue "v26 release not available via HomeBrew":
(https://github.com/bitcoin/bitcoin/issues/29190#issuecomment-1879017055)
> See [Homebrew/homebrew-core#156745](https://github.com/Homebrew/homebrew-core/pull/156745). That PR is currently blocked on test-failures in the brew (Linux) CI.
Thanks for the pointer!
(https://github.com/bitcoin/bitcoin/issues/29190#issuecomment-1879017055)
> See [Homebrew/homebrew-core#156745](https://github.com/Homebrew/homebrew-core/pull/156745). That PR is currently blocked on test-failures in the brew (Linux) CI.
Thanks for the pointer!
✅ so7ow closed an issue: "v26 release not available via HomeBrew"
(https://github.com/bitcoin/bitcoin/issues/29190)
(https://github.com/bitcoin/bitcoin/issues/29190)
👍 dergoegge approved a pull request: "fuzz: set `nMaxOutboundLimit` in connman target"
(https://github.com/bitcoin/bitcoin/pull/29172#pullrequestreview-1806452323)
utACK e5b9ee0221ec8aa238fb5720fcd6faa01b09fe46
(https://github.com/bitcoin/bitcoin/pull/29172#pullrequestreview-1806452323)
utACK e5b9ee0221ec8aa238fb5720fcd6faa01b09fe46
🤔 stickies-v reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1805846181)
Approach ACK, nice one finding this test case! 1a52ca7619eeb1baafa5a32b364381126862b29d looks good to me but not super familiar with this part of the code. Left some suggestions about maintainability and can be ignored.
I verified that the tests fail on #29019 as well as on 453b4813ebc74859864803e9972b58e4be76a4d6~1, as is necessary.
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1805846181)
Approach ACK, nice one finding this test case! 1a52ca7619eeb1baafa5a32b364381126862b29d looks good to me but not super familiar with this part of the code. Left some suggestions about maintainability and can be ignored.
I verified that the tests fail on #29019 as well as on 453b4813ebc74859864803e9972b58e4be76a4d6~1, as is necessary.
💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1442918332)
nit: makes more sense to unpack here imo
```suggestion
block_to_disconnect = self.generate(self.nodes[0], 1)[0]
```
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1442918332)
nit: makes more sense to unpack here imo
```suggestion
block_to_disconnect = self.generate(self.nodes[0], 1)[0]
```
💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1442808773)
nit: could be a bit more efficient by only calling and iterating over `listunspent` once (does not test for txid collisions, but I don't think we need that?)
<details>
<summary>git diff on 1a52ca7619</summary>
```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..928455c355 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -286,13 +286,12 @@ class ImportRescanTest(Bi
...
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1442808773)
nit: could be a bit more efficient by only calling and iterating over `listunspent` once (does not test for txid collisions, but I don't think we need that?)
<details>
<summary>git diff on 1a52ca7619</summary>
```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..928455c355 100755
--- a/test/functional/wallet_import_rescan.py
+++ b/test/functional/wallet_import_rescan.py
@@ -286,13 +286,12 @@ class ImportRescanTest(Bi
...
💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1443144137)
It doesn't seem like we're actually testing the expected balance at any point, rather the amount received, but I may be missing nuance. Would this diff make sense? I don't like adding an extra parameter to an already long function for something so specific, if we can avoid it.
<details>
<summary>git diff on 1a52ca7619</summary>
```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..fa9f43f66c 100755
--- a/test/functional
...
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1443144137)
It doesn't seem like we're actually testing the expected balance at any point, rather the amount received, but I may be missing nuance. Would this diff make sense? I don't like adding an extra parameter to an already long function for something so specific, if we can avoid it.
<details>
<summary>git diff on 1a52ca7619</summary>
```diff
diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py
index 1abc9c0c79..fa9f43f66c 100755
--- a/test/functional
...