⚠️ mzumsande opened an issue: "RfC: increase minimum prune target?"
(https://github.com/bitcoin/bitcoin/issues/30037)
The minimum pruning target of `550MiB` was chosen based on a physical block size of `1 MiB`, see https://github.com/bitcoin/bitcoin/blob/f5b6f621fff7540ca97974b26a66ca1cc6db018c/src/validation.h#L69-L76
We never prune files within 288 blocks from the tip, so the limit will not be respected if these blocks take up more space.
Since physical block sizes have increased since segwit, I think that there is no benefit to choosing `-prune=550` as opposed to `-prune=1000`:
Running with the smaller
...
(https://github.com/bitcoin/bitcoin/issues/30037)
The minimum pruning target of `550MiB` was chosen based on a physical block size of `1 MiB`, see https://github.com/bitcoin/bitcoin/blob/f5b6f621fff7540ca97974b26a66ca1cc6db018c/src/validation.h#L69-L76
We never prune files within 288 blocks from the tip, so the limit will not be respected if these blocks take up more space.
Since physical block sizes have increased since segwit, I think that there is no benefit to choosing `-prune=550` as opposed to `-prune=1000`:
Running with the smaller
...
🤔 tdb3 reviewed a pull request: "test: Handle functional test disk-full error"
(https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-2038975683)
re ACK for 357ad110548d726021547d85b5b2bfcf3191d7e3
Retested https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-1999297020 (and received the same results, which warn the user of insufficient free space and early exit due to insufficient space).
(https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-2038975683)
re ACK for 357ad110548d726021547d85b5b2bfcf3191d7e3
Retested https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-1999297020 (and received the same results, which warn the user of insufficient free space and early exit due to insufficient space).
🤔 tdb3 reviewed a pull request: "rpc: return warnings as an array instead of just a single one"
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-2039013213)
Re ACK for 42fb5311b19582361409d65c6fddeadbee14bb97
Ran the tests in https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2081107100 and https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-1999801519 again (no warnings, one warning, two warnings, and with `-deprecatedrpc` with one warning). All behaved as expected.
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-2039013213)
Re ACK for 42fb5311b19582361409d65c6fddeadbee14bb97
Ran the tests in https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2081107100 and https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-1999801519 again (no warnings, one warning, two warnings, and with `-deprecatedrpc` with one warning). All behaved as expected.
💬 brunoerg commented on issue "Possible to Ban Clients by Name?":
(https://github.com/bitcoin/bitcoin/issues/30036#issuecomment-2093786404)
I don't think it would be effective. If we implemented something like it and, if they're bad/malicious peers, they can just vary it and bypass this ban.
(https://github.com/bitcoin/bitcoin/issues/30036#issuecomment-2093786404)
I don't think it would be effective. If we implemented something like it and, if they're bad/malicious peers, they can just vary it and bypass this ban.
✅ brunoerg closed an issue: "wallet, coin selection: config option to prioritize 'no change' when possible"
(https://github.com/bitcoin/bitcoin/issues/23372)
(https://github.com/bitcoin/bitcoin/issues/23372)
💬 kevkevinpal commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1589760663)
that is fair I initially did it like this because there was one scenario where we used two `<` operators but I now switched to using `assert_greater_than` and now broke that statement up to two statements
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1589760663)
that is fair I initially did it like this because there was one scenario where we used two `<` operators but I now switched to using `assert_greater_than` and now broke that statement up to two statements
💬 kevkevinpal commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#issuecomment-2093824753)
pushed to [fe07516](https://github.com/bitcoin/bitcoin/pull/30019/commits/fe075160f04c3e57fab9571891b17a4ce9f8bec3)
we now use `assert_greater_than` instead of creating a new util
[04d8f07](https://github.com/bitcoin/bitcoin/pull/30019/commits/04d8f07c4317ec4c517ae060a1c2fea2b01416df) contains a scripted-diff which looks for `assert.*< ` and then swaps the two values and replaces the `assert` with `assert_greater_than`
(https://github.com/bitcoin/bitcoin/pull/30019#issuecomment-2093824753)
pushed to [fe07516](https://github.com/bitcoin/bitcoin/pull/30019/commits/fe075160f04c3e57fab9571891b17a4ce9f8bec3)
we now use `assert_greater_than` instead of creating a new util
[04d8f07](https://github.com/bitcoin/bitcoin/pull/30019/commits/04d8f07c4317ec4c517ae060a1c2fea2b01416df) contains a scripted-diff which looks for `assert.*< ` and then swaps the two values and replaces the `assert` with `assert_greater_than`
💬 davidgumberg commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1589807191)
Nit: IMO, the length check should happen before the `IsHex` check, since a string with an odd-numbered length of valid hex characters will fail with the slightly confusing error message that it "must be a hex string." For example, 67 zeroes:
```console
$ bitcoin-cli importpubkey 0000000000000000000000000000000000000000000000000000000000000000000
error code: -5
error message:
Pubkey "0000000000000000000000000000000000000000000000000000000000000000000" must be a hex string
```
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1589807191)
Nit: IMO, the length check should happen before the `IsHex` check, since a string with an odd-numbered length of valid hex characters will fail with the slightly confusing error message that it "must be a hex string." For example, 67 zeroes:
```console
$ bitcoin-cli importpubkey 0000000000000000000000000000000000000000000000000000000000000000000
error code: -5
error message:
Pubkey "0000000000000000000000000000000000000000000000000000000000000000000" must be a hex string
```
💬 davidgumberg commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1589810999)
Nit-question: Should there be a test added here that checks for pubkeys that are not valid points as in https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e?
Or, since this PR deduplicates pubkey parsing, is this test of the length check redundant and worthy of being removed?
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1589810999)
Nit-question: Should there be a test added here that checks for pubkeys that are not valid points as in https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e?
Or, since this PR deduplicates pubkey parsing, is this test of the length check redundant and worthy of being removed?
💬 davidgumberg commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1589811146)
see: https://github.com/bitcoin/bitcoin/pull/28336/commits/100e8a75bf5d8196c005331bd8f2ed42ada6d8d0#r1589810999
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1589811146)
see: https://github.com/bitcoin/bitcoin/pull/28336/commits/100e8a75bf5d8196c005331bd8f2ed42ada6d8d0#r1589810999
💬 davidgumberg commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#issuecomment-2093897482)
ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
The deduplication of pubkey parsing here is a big improvement, and granular pubkey error messages make the rpc interface easier to use. As far as I could find, no instances of pubkey parsing in the rpc code were missed in this PR.
I tested this PR manually by compiling with legacy wallet support and passing bad pubkeys with the `importpubkey` rpc.
(https://github.com/bitcoin/bitcoin/pull/28336#issuecomment-2093897482)
ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
The deduplication of pubkey parsing here is a big improvement, and granular pubkey error messages make the rpc interface easier to use. As far as I could find, no instances of pubkey parsing in the rpc code were missed in this PR.
I tested this PR manually by compiling with legacy wallet support and passing bad pubkeys with the `importpubkey` rpc.
💬 fanquake commented on pull request "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set":
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2093918590)
> Would it make sense to mention that -Wno-error=... should be passed, when needed, in the compile docs?
Maybe into the CI or dev docs, given we'll never use -Werror by default for compilation?
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-2093918590)
> Would it make sense to mention that -Wno-error=... should be passed, when needed, in the compile docs?
Maybe into the CI or dev docs, given we'll never use -Werror by default for compilation?
✅ fanquake closed an issue: "build: CXXFLAGS with depends"
(https://github.com/bitcoin/bitcoin/issues/18092)
(https://github.com/bitcoin/bitcoin/issues/18092)
🚀 fanquake merged a pull request: "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set"
(https://github.com/bitcoin/bitcoin/pull/25972)
(https://github.com/bitcoin/bitcoin/pull/25972)
✅ fanquake closed an issue: "`test/streams_tests.cpp` fails to compile on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/issues/29884)
(https://github.com/bitcoin/bitcoin/issues/29884)
🚀 fanquake merged a pull request: "test: Fix `test/streams_tests.cpp` compilation on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/pull/29907)
(https://github.com/bitcoin/bitcoin/pull/29907)
🚀 fanquake merged a pull request: "miniscript: make operator""_mst consteval"
(https://github.com/bitcoin/bitcoin/pull/28657)
(https://github.com/bitcoin/bitcoin/pull/28657)
💬 fanquake commented on pull request "msvc: Compile `test\fuzz\miniscript.cpp`":
(https://github.com/bitcoin/bitcoin/pull/30031#issuecomment-2093940009)
> Based on https://github.com/bitcoin/bitcoin/pull/28657.
(https://github.com/bitcoin/bitcoin/pull/30031#issuecomment-2093940009)
> Based on https://github.com/bitcoin/bitcoin/pull/28657.
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2093953949)
> We made `acceptnonstdtxn=0` the default for testnet3 in #28354. We could flip it back on. Or we could make it easier to preferentially peer with no-standardness nodes (and miners) to get those in. I don't think relying on low difficulty blocks is the right solution, at least I don't think it's a good enough reason to keep them around.
Yeah, I tend to agree, I thought about flipping it back on earlier as well: https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2041139129 but we weren
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2093953949)
> We made `acceptnonstdtxn=0` the default for testnet3 in #28354. We could flip it back on. Or we could make it easier to preferentially peer with no-standardness nodes (and miners) to get those in. I don't think relying on low difficulty blocks is the right solution, at least I don't think it's a good enough reason to keep them around.
Yeah, I tend to agree, I thought about flipping it back on earlier as well: https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2041139129 but we weren
...
💬 fanquake commented on pull request "guix: build with glibc 2.31":
(https://github.com/bitcoin/bitcoin/pull/29987#issuecomment-2093954870)
Rebased and pulled in 1 more commit from the 2.31 branch.
(https://github.com/bitcoin/bitcoin/pull/29987#issuecomment-2093954870)
Rebased and pulled in 1 more commit from the 2.31 branch.