💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818882978)
042a97ce7fc672021cdb1dee62a550ef19c208fb
code-wise i think this better belongs around `MakeAndPushMessage` (what if the caller decides to postpone the actual sending), but no big deal.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818882978)
042a97ce7fc672021cdb1dee62a550ef19c208fb
code-wise i think this better belongs around `MakeAndPushMessage` (what if the caller decides to postpone the actual sending), but no big deal.
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818893833)
Okay, so
` /** New inv has been received. May be added as a candidate to txrequest. */` is inaccurate, right? There is no new INV received in `p2p_inv=false` case, but rather it's called from orphan consideration after processing a TX?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818893833)
Okay, so
` /** New inv has been received. May be added as a candidate to txrequest. */` is inaccurate, right? There is no new INV received in `p2p_inv=false` case, but rather it's called from orphan consideration after processing a TX?
👍 TheCharlatan approved a pull request: "build: Rename `PACKAGE_*` variables to `CLIENT_*`"
(https://github.com/bitcoin/bitcoin/pull/31042#pullrequestreview-2399049784)
Re-ACK 70713303b6399e0627c1960da4fbab48f9cf71e7
(https://github.com/bitcoin/bitcoin/pull/31042#pullrequestreview-2399049784)
Re-ACK 70713303b6399e0627c1960da4fbab48f9cf71e7
🤔 stickies-v reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2393162554)
Note: `doc/bips.md` needs updating, "opt-in" is no longer accurate: https://github.com/bitcoin/bitcoin/blob/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001/doc/bips.md?plain=1#L37
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2393162554)
Note: `doc/bips.md` needs updating, "opt-in" is no longer accurate: https://github.com/bitcoin/bitcoin/blob/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001/doc/bips.md?plain=1#L37
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819051835)
Keeping this section here feels unnecessarily confusing to me. Since it's no longer relevant, I'd just remove the entire paragraph, and potentially add a line in the `## History` section?
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819051835)
Keeping this section here feels unnecessarily confusing to me. Since it's no longer relevant, I'd just remove the entire paragraph, and potentially add a line in the `## History` section?
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1818979310)
nit: suggestion to provide a bit more context. Ideally, at least the PR number is added.
```suggestion
Starting with v28.0, the `mempoolfullrbf` startup option was set to
default to `1`. With widespread adoption of this policy, users no longer
benefit from disabling it, so the option has been removed, making full
replace-by-fee the standard behavior. (#30592)
```
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1818979310)
nit: suggestion to provide a bit more context. Ideally, at least the PR number is added.
```suggestion
Starting with v28.0, the `mempoolfullrbf` startup option was set to
default to `1`. With widespread adoption of this policy, users no longer
benefit from disabling it, so the option has been removed, making full
replace-by-fee the standard behavior. (#30592)
```
💬 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.