💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2240599386)
re: https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2232487097
> Are you still working on this, or do you want someone to pick it up?
I would like code review, and I probably need to respond to some old comments and questions. There are two parts to this PR:
- The first part of this PR, implemented in the first commit, uses the ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags to validate command line and config file settings, and save them into UniValue values as typed values, r
...
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2240599386)
re: https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2232487097
> Are you still working on this, or do you want someone to pick it up?
I would like code review, and I probably need to respond to some old comments and questions. There are two parts to this PR:
- The first part of this PR, implemented in the first commit, uses the ALLOW_BOOL, ALLOW_INT, and ALLOW_STRING flags to validate command line and config file settings, and save them into UniValue values as typed values, r
...
💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1685163279)
This diff will gone after https://github.com/hebasto/bitcoin/pull/272.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1685163279)
This diff will gone after https://github.com/hebasto/bitcoin/pull/272.
💬 hebasto commented on issue "bump boost test to 1.59?":
(https://github.com/bitcoin/bitcoin/issues/19128#issuecomment-2240787323)
> To skip tests.
See https://github.com/hebasto/bitcoin/pull/272.
(https://github.com/bitcoin/bitcoin/issues/19128#issuecomment-2240787323)
> To skip tests.
See https://github.com/hebasto/bitcoin/pull/272.
📝 FisZika opened a pull request: "Update translation_process.md"
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...
✅ achow101 closed a pull request: "Update translation_process.md"
(https://github.com/bitcoin/bitcoin/pull/30492)
(https://github.com/bitcoin/bitcoin/pull/30492)
📝 achow101 locked a pull request: "Update translation_process.md"
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...
(https://github.com/bitcoin/bitcoin/pull/30492)
B5690EEEBB952194
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or n
...
📝 1440000bytes opened a pull request: "policy: enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/30493)
This pull request enables full rbf (mempool policy) by default. Commits are same as #28132 however it was closed recently with this [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2225369634).
---
Rationale:
- Full RBF config option was added in July 2022: https://github.com/bitcoin/bitcoin/pull/25353
- It is used regularly: https://mempool.space/rbf#fullrbf
- Most mining pools are using it: https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2059120917
...
(https://github.com/bitcoin/bitcoin/pull/30493)
This pull request enables full rbf (mempool policy) by default. Commits are same as #28132 however it was closed recently with this [comment](https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2225369634).
---
Rationale:
- Full RBF config option was added in July 2022: https://github.com/bitcoin/bitcoin/pull/25353
- It is used regularly: https://mempool.space/rbf#fullrbf
- Most mining pools are using it: https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2059120917
...
💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1685289194)
Thanks! Fixed in https://github.com/hebasto/bitcoin/pull/273.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1685289194)
Thanks! Fixed in https://github.com/hebasto/bitcoin/pull/273.
💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2240971961)
Temporarily reopening this PR to demonstrate the correctness of the https://github.com/hebasto/bitcoin/pull/273.
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2240971961)
Temporarily reopening this PR to demonstrate the correctness of the https://github.com/hebasto/bitcoin/pull/273.
📝 hebasto reopened a pull request: "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP"
(https://github.com/bitcoin/bitcoin/pull/29790)
This PR goal is to facilitate testing CI scripts migration to the CMake.
(https://github.com/bitcoin/bitcoin/pull/29790)
This PR goal is to facilitate testing CI scripts migration to the CMake.
💬 BitcoinErrorLog commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241046005)
mempoolfullrbf should be removed, and RBF is a redundant unenforceable feature. If we believe miners only care about the the largest fee/rate then simply providing them with all txns is good enough. Miners can choose how to sort themselves.
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241046005)
mempoolfullrbf should be removed, and RBF is a redundant unenforceable feature. If we believe miners only care about the the largest fee/rate then simply providing them with all txns is good enough. Miners can choose how to sort themselves.
💬 hebasto commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2241046345)
> There are a few things that `--enable-fuzz` does that we definitely want it to do:
>
> * As you said, this option disables all other binaries besides the `fuzz` binary. This is nice when you only want to do fuzzing and nothing else (e.g. oss-fuzz). It is also required for the use of libFuzzer (`--with-sanitizers=fuzzer` i.e. `-fsanitize=fuzzer` with clang), because the other binaries provide a main function.
This functionality does not require any build options. The fuzz binary can be
...
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2241046345)
> There are a few things that `--enable-fuzz` does that we definitely want it to do:
>
> * As you said, this option disables all other binaries besides the `fuzz` binary. This is nice when you only want to do fuzzing and nothing else (e.g. oss-fuzz). It is also required for the use of libFuzzer (`--with-sanitizers=fuzzer` i.e. `-fsanitize=fuzzer` with clang), because the other binaries provide a main function.
This functionality does not require any build options. The fuzz binary can be
...
💬 jrakibi commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2241097572)
ACK [d63ef73](https://github.com/bitcoin/bitcoin/commit/d63ef738001fb69ce04134cc8645dcd1e1cbccd1)
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2241097572)
ACK [d63ef73](https://github.com/bitcoin/bitcoin/commit/d63ef738001fb69ce04134cc8645dcd1e1cbccd1)
💬 andrewtoth commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241100701)
> If we believe miners only care about the the largest fee/rate then simply providing them with all txns is good enough. Miners can choose how to sort themselves.
This patch is necessary to provide miners with all txns. As of now if a node running default settings is between you and a miner, and you send it a txn not flagged as rbf enabled, that node will not relay another txn spending the same inputs as the first txn to the miner even if it pays a larger fee and fee rate. The miner will not be
...
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241100701)
> If we believe miners only care about the the largest fee/rate then simply providing them with all txns is good enough. Miners can choose how to sort themselves.
This patch is necessary to provide miners with all txns. As of now if a node running default settings is between you and a miner, and you send it a txn not flagged as rbf enabled, that node will not relay another txn spending the same inputs as the first txn to the miner even if it pays a larger fee and fee rate. The miner will not be
...
💬 tdb3 commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241107700)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241107700)
Concept ACK
🚀 fanquake merged a pull request: "depends: bump libmultiprocess for CMake fixes"
(https://github.com/bitcoin/bitcoin/pull/30490)
(https://github.com/bitcoin/bitcoin/pull/30490)
💬 fanquake commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2241136660)
Dropped the (autotools) zeromq commit and rebased on top of #29723.
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2241136660)
Dropped the (autotools) zeromq commit and rebased on top of #29723.
💬 fanquake commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2241142027)
Guix build (aarch64):
```bash
5447a6b0ed155318747354b9db7972716ba5a5c45c956ca39e979c380fe7b25c guix-build-298c8192a71e/output/arm64-apple-darwin/SHA256SUMS.part
9bc5df1ade9d2a8d9c469ffcbc72ff064c57658474a028270fbeeb4fafb7cff4 guix-build-298c8192a71e/output/arm64-apple-darwin/bitcoin-298c8192a71e-arm64-apple-darwin-unsigned.tar.gz
7aa774a9731f69379ae994a60c7fef3e33d07ebcb5f70b0cc0224e0640f638e2 guix-build-298c8192a71e/output/arm64-apple-darwin/bitcoin-298c8192a71e-arm64-apple-darwin-unsign
...
(https://github.com/bitcoin/bitcoin/pull/30451#issuecomment-2241142027)
Guix build (aarch64):
```bash
5447a6b0ed155318747354b9db7972716ba5a5c45c956ca39e979c380fe7b25c guix-build-298c8192a71e/output/arm64-apple-darwin/SHA256SUMS.part
9bc5df1ade9d2a8d9c469ffcbc72ff064c57658474a028270fbeeb4fafb7cff4 guix-build-298c8192a71e/output/arm64-apple-darwin/bitcoin-298c8192a71e-arm64-apple-darwin-unsigned.tar.gz
7aa774a9731f69379ae994a60c7fef3e33d07ebcb5f70b0cc0224e0640f638e2 guix-build-298c8192a71e/output/arm64-apple-darwin/bitcoin-298c8192a71e-arm64-apple-darwin-unsign
...
💬 hebasto commented on pull request "depends: bump libmultiprocess for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30490#issuecomment-2241153244)
> Presumably this approach works now with the CMake branch?
Unfortunately, it doesn't.
The PR30454 [branch](https://github.com/hebasto/bitcoin/tree/240716-cmake) rebased on the master branch @ efeb39785aeee9130584b865d886c6b46e59f147 fails:
```
$ make -C depends NO_QT=1 NO_WALLET=1 NO_UPNP=1 NO_NATPMP=1 NO_ZMQ=1 NO_USDT=1 MULTIPROCESS=1
$ cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake
...
CMake Error at CMakeLists.txt:158 (find_package):
Could not find a pac
...
(https://github.com/bitcoin/bitcoin/pull/30490#issuecomment-2241153244)
> Presumably this approach works now with the CMake branch?
Unfortunately, it doesn't.
The PR30454 [branch](https://github.com/hebasto/bitcoin/tree/240716-cmake) rebased on the master branch @ efeb39785aeee9130584b865d886c6b46e59f147 fails:
```
$ make -C depends NO_QT=1 NO_WALLET=1 NO_UPNP=1 NO_NATPMP=1 NO_ZMQ=1 NO_USDT=1 MULTIPROCESS=1
$ cmake -B build --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake
...
CMake Error at CMakeLists.txt:158 (find_package):
Could not find a pac
...
💬 fanquake commented on pull request "Fix MSVC warning C4273 "inconsistent dll linkage"":
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2241157914)
> Though, I don't like the slippery slope notion here. Should we go ahead and disable runtimes/link modes we don't support? Just because CMake enables it doesn't mean we want to start supporting it.
I agree. It seems that rather than just porting what we currently build for MSVC, and adding support for new things later (once we've decided what to do), we've instead ported master + assumed support for a number of other things that we don't otherwise test/maintain, or use to ship releases.
J
...
(https://github.com/bitcoin/bitcoin/pull/30491#issuecomment-2241157914)
> Though, I don't like the slippery slope notion here. Should we go ahead and disable runtimes/link modes we don't support? Just because CMake enables it doesn't mean we want to start supporting it.
I agree. It seems that rather than just porting what we currently build for MSVC, and adding support for new things later (once we've decided what to do), we've instead ported master + assumed support for a number of other things that we don't otherwise test/maintain, or use to ship releases.
J
...