💬 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
...
🚀 fanquake merged a pull request: "Fix MSVC warning C4273 "inconsistent dll linkage""
(https://github.com/bitcoin/bitcoin/pull/30491)
(https://github.com/bitcoin/bitcoin/pull/30491)
💬 reardencode commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241158440)
ACK
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241158440)
ACK
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875)
I'm fairly confident this is not a behavior change, but could have missed something.
If you look at `secp256k1_keypair_xonly_pub`, this will only fail if there was an issue creating the `keypair` object, but this is checked above so we would have already short-circuited. The same is true for `secp256k1_keypair_xonly_pubkey_serialize` (only fails if we can't load the `keypair`, which only fails if the `keypair` is malformed).
`secp256k1_keypair_xonly_tweak_add` will fail if `tweak32` is gr
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875)
I'm fairly confident this is not a behavior change, but could have missed something.
If you look at `secp256k1_keypair_xonly_pub`, this will only fail if there was an issue creating the `keypair` object, but this is checked above so we would have already short-circuited. The same is true for `secp256k1_keypair_xonly_pubkey_serialize` (only fails if we can't load the `keypair`, which only fails if the `keypair` is malformed).
`secp256k1_keypair_xonly_tweak_add` will fail if `tweak32` is gr
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685450966)
Personally, I don't see any reason to not reuse `ret` and I think it makes the logic easier to follow vs re-initializing the variable. Regarding returning inside the `if (ret)`, as mentioned in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875, `keypair_xonly_pub` will only fail if the `keypair` is malformed, so if we've successfully created the keypair this call should never fail. So if `schnorrsig_verify` fails, we check it on the next line and clear the signature data befor
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685450966)
Personally, I don't see any reason to not reuse `ret` and I think it makes the logic easier to follow vs re-initializing the variable. Regarding returning inside the `if (ret)`, as mentioned in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875, `keypair_xonly_pub` will only fail if the `keypair` is malformed, so if we've successfully created the keypair this call should never fail. So if `schnorrsig_verify` fails, we check it on the next line and clear the signature data befor
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685454366)
Good catch, I think you're right that if `xonly_tweak_add` were to fail (e.g. tweak32 is the negation of the secret key), then we would return without explicitly clearing the keypair variable. That being said, the variable should already be cleared when it goes out of scope, so the explicit `memory_cleanse` is more of a belt and suspenders approach, from what I understand.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685454366)
Good catch, I think you're right that if `xonly_tweak_add` were to fail (e.g. tweak32 is the negation of the secret key), then we would return without explicitly clearing the keypair variable. That being said, the variable should already be cleared when it goes out of scope, so the explicit `memory_cleanse` is more of a belt and suspenders approach, from what I understand.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685459510)
Hm, I don't think it makes much of a difference for the diff and makes this particular staging commit more confusing by creating the `KeyPair` object just to pull out the data back out into a `secp256k1_keypair` object. The main advantage of the `KeyPair` wrapper class is that we are keeping the keypair in secure memory, which we would be undoing here by copying it out into a `keypair` object.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685459510)
Hm, I don't think it makes much of a difference for the diff and makes this particular staging commit more confusing by creating the `KeyPair` object just to pull out the data back out into a `secp256k1_keypair` object. The main advantage of the `KeyPair` wrapper class is that we are keeping the keypair in secure memory, which we would be undoing here by copying it out into a `keypair` object.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685460212)
`secp256k1_keypair_xonly_tweak_add` modifies the `keypair` object with `tweak32`, so it can't be const. More generally, libsecp256k1 is a dependency of Bitcoin Core, so we shouldn't ever be modifying `secp256k1_` functions in our code base (despite us pulling it in as a subtree).
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685460212)
`secp256k1_keypair_xonly_tweak_add` modifies the `keypair` object with `tweak32`, so it can't be const. More generally, libsecp256k1 is a dependency of Bitcoin Core, so we shouldn't ever be modifying `secp256k1_` functions in our code base (despite us pulling it in as a subtree).
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685462749)
`IsValid()` returns false when `m_keypair` data is a nullptr, which effectively means the key data was never created or has been cleared (see `src/support/allocators/secure.h`)
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685462749)
`IsValid()` returns false when `m_keypair` data is a nullptr, which effectively means the key data was never created or has been cleared (see `src/support/allocators/secure.h`)
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685467250)
> Since keypair is a local variable in this function shouldn't the KeyPair destructor do the secure erase job anyway at the end of the scope through RAII since m_keypair is a secure_unique_ptr
Correct, which should happen regardless of whether or not we explicitly clear the key data with `keypair = KeyPair();`. Having `keypair = KeyPair()` a belt and suspenders by explicitly clearing the variable. This is because if we create a `KeyPair` object with the default constructor, there is no call t
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685467250)
> Since keypair is a local variable in this function shouldn't the KeyPair destructor do the secure erase job anyway at the end of the scope through RAII since m_keypair is a secure_unique_ptr
Correct, which should happen regardless of whether or not we explicitly clear the key data with `keypair = KeyPair();`. Having `keypair = KeyPair()` a belt and suspenders by explicitly clearing the variable. This is because if we create a `KeyPair` object with the default constructor, there is no call t
...