💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1815326806)
nit: would just drop this line (+newline entirely), already covered in `self.log.info('A transaction that replaces a mempool transaction')`
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1815326806)
nit: would just drop this line (+newline entirely), already covered in `self.log.info('A transaction that replaces a mempool transaction')`
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819055432)
(note: this did not get fixed - it's a trivial re-ack, would be ideal to get this in this PR - it's very confusing when reading the code)
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819055432)
(note: this did not get fixed - it's a trivial re-ack, would be ideal to get this in this PR - it's very confusing when reading the code)
💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819093318)
Misunderstood the code at time of writing this comment, mark as resolved
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819093318)
Misunderstood the code at time of writing this comment, mark as resolved
👍 stickies-v approved a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2399208958)
ACK d295638890719a009b480148545288e2dbdd1a5a - although I'll quickly re-ACK if you address my [outstanding comments](https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2393162554) too.
Nice clean-up, both for users and the codebase, I don't see the point of keeping this option around any longer.
> if one can go and publish on the mailing list in which core release this is expected to land and indicates the expected deployment timelines for the downstream softwares.
I don't
...
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2399208958)
ACK d295638890719a009b480148545288e2dbdd1a5a - although I'll quickly re-ACK if you address my [outstanding comments](https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2393162554) too.
Nice clean-up, both for users and the codebase, I don't see the point of keeping this option around any longer.
> if one can go and publish on the mailing list in which core release this is expected to land and indicates the expected deployment timelines for the downstream softwares.
I don't
...
💬 pinheadmz commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819160027)
Sure I'll use `ToString()` here. Going to leave the hyphens though because there are several other hyphenated fields in the response object already... but I do see lots of underscores in other RPC responses 😬
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819160027)
Sure I'll use `ToString()` here. Going to leave the hyphens though because there are several other hyphenated fields in the response object already... but I do see lots of underscores in other RPC responses 😬
💬 pinheadmz commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819194151)
fixed thanks
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819194151)
fixed thanks
💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819197161)
This is updated -- does this seem sufficient now, or should I add more documentation about how reorgs work?
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819197161)
This is updated -- does this seem sufficient now, or should I add more documentation about how reorgs work?
💬 hebasto commented on pull request "depends: Specify CMake generator explicitly":
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2441827065)
> > However, this assumption can be wrong in environments where the [CMAKE_GENERATOR](https://cmake.org/cmake/help/latest/envvar/CMAKE_GENERATOR.html) variable is set.
>
> Generally, this holds for many CMake variables. Wouldn't a better approach be facilitating it working, rather than guaranteeing these environments never get the expected bahaviour.
Such behaviour can be achieved by switching from `$(MAKE)` to `cmake --build ...` in the `$(package)_build_cmds`. However, it would be a much
...
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2441827065)
> > However, this assumption can be wrong in environments where the [CMAKE_GENERATOR](https://cmake.org/cmake/help/latest/envvar/CMAKE_GENERATOR.html) variable is set.
>
> Generally, this holds for many CMake variables. Wouldn't a better approach be facilitating it working, rather than guaranteeing these environments never get the expected bahaviour.
Such behaviour can be achieved by switching from `$(MAKE)` to `cmake --build ...` in the `$(package)_build_cmds`. However, it would be a much
...
💬 jonatack commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819230951)
> Can be ignored imo, it'd be deleted once copied to release notes draft anyway
Yes, my understanding is that these serve to indicate where to insert the release note.
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819230951)
> Can be ignored imo, it'd be deleted once copied to release notes draft anyway
Yes, my understanding is that these serve to indicate where to insert the release note.
📝 hebasto opened a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173)
This PR introduces the `FindQRencode` module, which is capable of finding the vcpkg [`libqrencode`](https://github.com/microsoft/vcpkg/blob/c8582b4d83dbd36e1bebc08bf166b5eb807996b0/ports/libqrencode/usage) package.
In regard to https://github.com/bitcoin/bitcoin/issues/30876, please note that the [upstream project](https://github.com/fukuchi/libqrencode) does not provide CMake config files.
Additionally, the `libqrencode` vcpkg package is enabled for MSVC builds.
Here is a diff for conf
...
(https://github.com/bitcoin/bitcoin/pull/31173)
This PR introduces the `FindQRencode` module, which is capable of finding the vcpkg [`libqrencode`](https://github.com/microsoft/vcpkg/blob/c8582b4d83dbd36e1bebc08bf166b5eb807996b0/ports/libqrencode/usage) package.
In regard to https://github.com/bitcoin/bitcoin/issues/30876, please note that the [upstream project](https://github.com/fukuchi/libqrencode) does not provide CMake config files.
Additionally, the `libqrencode` vcpkg package is enabled for MSVC builds.
Here is a diff for conf
...
💬 pinheadmz commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819233268)
So actually, after making this change I discovered several tests fail everywhere `reject-reason` is checked. There will be a lot of diffs like this... let me know if this changes your mind at all about that `if` condition there.
```diff
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -109,7 +109,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
))
# ... 0.99 passes
self.check_mempool_result(
- result_expecte
...
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819233268)
So actually, after making this change I discovered several tests fail everywhere `reject-reason` is checked. There will be a lot of diffs like this... let me know if this changes your mind at all about that `if` condition there.
```diff
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -109,7 +109,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
))
# ... 0.99 passes
self.check_mempool_result(
- result_expecte
...
💬 jonatack commented on pull request "include verbose "reject-details" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819237087)
> Going to leave the hyphens though because there are several other hyphenated fields in the response object already... but I do see lots of underscores in other RPC responses 😬
Yes, per our RPC interface guidelines in the developer notes:
```md
- Argument and field naming: please consider whether there is already a naming
style or spelling convention in the API for the type of object in question
(`blockhash`, for example), and if so, try to use that. If not, use snake case
`fee
...
(https://github.com/bitcoin/bitcoin/pull/28121#discussion_r1819237087)
> Going to leave the hyphens though because there are several other hyphenated fields in the response object already... but I do see lots of underscores in other RPC responses 😬
Yes, per our RPC interface guidelines in the developer notes:
```md
- Argument and field naming: please consider whether there is already a naming
style or spelling convention in the API for the type of object in question
(`blockhash`, for example), and if so, try to use that. If not, use snake case
`fee
...
💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1819257822)
I think so. Using this, I see empty service names while a peer connection is setting up, and often from one or two (usually inbound tor) peers. It looks like `NODE_NONE` would be returned as an empty string by `serviceFlagsToStr()`.
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1819257822)
I think so. Using this, I see empty service names while a peer connection is setting up, and often from one or two (usually inbound tor) peers. It looks like `NODE_NONE` would be returned as an empty string by `serviceFlagsToStr()`.
🤔 fanquake reviewed a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2399404041)
Concept ACK
> --- Checking for module 'libqrencode'
> --- Found libqrencode, version 4.1.1
> +-- Found QRencode: /usr/lib/x86_64-linux-gnu/libqrencode.so
This output seems less useful, given it no-longer has the version.
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2399404041)
Concept ACK
> --- Checking for module 'libqrencode'
> --- Found libqrencode, version 4.1.1
> +-- Found QRencode: /usr/lib/x86_64-linux-gnu/libqrencode.so
This output seems less useful, given it no-longer has the version.
💬 fanquake commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#discussion_r1819254887)
> various build environments
Given that all the new code is just to facilitate MSVC/`vcpkg`, you could probably be a bit more concrete with the documentation. Otherwise someone might wonder why all of this is being done, when
```bash
find_package(PkgConfig REQUIRED)
pkg_check_modules(PC_QRencode QUIET libqrencode)
```
would otherwise work.
(https://github.com/bitcoin/bitcoin/pull/31173#discussion_r1819254887)
> various build environments
Given that all the new code is just to facilitate MSVC/`vcpkg`, you could probably be a bit more concrete with the documentation. Otherwise someone might wonder why all of this is being done, when
```bash
find_package(PkgConfig REQUIRED)
pkg_check_modules(PC_QRencode QUIET libqrencode)
```
would otherwise work.
💬 fanquake commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2441896956)
Guix Build:
```bash
2df1f3823c8f689140e3a400f0fdc49d4fe0d2bc4d29402d9765bf9821147b28 guix-build-70713303b639/output/aarch64-linux-gnu/SHA256SUMS.part
33a4689e3200b0fbd56d8098b33cf9388baebe641d27dd6dc09813f56ea2abb1 guix-build-70713303b639/output/aarch64-linux-gnu/bitcoin-70713303b639-aarch64-linux-gnu-debug.tar.gz
35f43ddc81853d054544bbe70d7d8461867c026a7b4d3c9ae90ee248bf28c9de guix-build-70713303b639/output/aarch64-linux-gnu/bitcoin-70713303b639-aarch64-linux-gnu.tar.gz
b87c9dd699093402
...
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2441896956)
Guix Build:
```bash
2df1f3823c8f689140e3a400f0fdc49d4fe0d2bc4d29402d9765bf9821147b28 guix-build-70713303b639/output/aarch64-linux-gnu/SHA256SUMS.part
33a4689e3200b0fbd56d8098b33cf9388baebe641d27dd6dc09813f56ea2abb1 guix-build-70713303b639/output/aarch64-linux-gnu/bitcoin-70713303b639-aarch64-linux-gnu-debug.tar.gz
35f43ddc81853d054544bbe70d7d8461867c026a7b4d3c9ae90ee248bf28c9de guix-build-70713303b639/output/aarch64-linux-gnu/bitcoin-70713303b639-aarch64-linux-gnu.tar.gz
b87c9dd699093402
...
💬 hebasto commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2441904898)
Tested 49994fadb3b71552fd99acd8671c52e08284b5e9.
The cross-compiled on Ubuntu 24.10 `bitcoind.exe` fails to run on Windows 11 Pro 23H2.
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2441904898)
Tested 49994fadb3b71552fd99acd8671c52e08284b5e9.
The cross-compiled on Ubuntu 24.10 `bitcoind.exe` fails to run on Windows 11 Pro 23H2.
💬 sr-gi commented on pull request "test: fix intermittent failure in p2p_seednode.py, don't connect to random IPs":
(https://github.com/bitcoin/bitcoin/pull/31142#issuecomment-2441921060)
tACK [6c9fe7b](https://github.com/bitcoin/bitcoin/pull/31142/commits/6c9fe7b73ea1572b8b56c716ab13d9866f91c6e9)
(https://github.com/bitcoin/bitcoin/pull/31142#issuecomment-2441921060)
tACK [6c9fe7b](https://github.com/bitcoin/bitcoin/pull/31142/commits/6c9fe7b73ea1572b8b56c716ab13d9866f91c6e9)
💬 fanquake commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2441921131)
> The cross-compiled on Ubuntu 24.10 bitcoind.exe fails to run on Windows 11 Pro 23H2.
Can you provide some actionable information? The CI and cross-compiled unit tests have run & passed.
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2441921131)
> The cross-compiled on Ubuntu 24.10 bitcoind.exe fails to run on Windows 11 Pro 23H2.
Can you provide some actionable information? The CI and cross-compiled unit tests have run & passed.
💬 mzumsande commented on pull request "test: Don't enforce BIP94 on regtest unless specified by arg":
(https://github.com/bitcoin/bitcoin/pull/31156#discussion_r1819302834)
good point, I forgot about `-test=`. Changed it to that approach, as a a result a `regtest` prefix is no longer necessary because the `-test=` will fail init on other networks anyway.
(https://github.com/bitcoin/bitcoin/pull/31156#discussion_r1819302834)
good point, I forgot about `-test=`. Changed it to that approach, as a a result a `regtest` prefix is no longer necessary because the `-test=` will fail init on other networks anyway.