💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1447076053)
But we don't do this much anyway?
I think the code could be easily modified for this change, you'd only have to pass addr direction to `AddWhitelistPermissionFlags. I won't insist though.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1447076053)
But we don't do this much anyway?
I think the code could be easily modified for this change, you'd only have to pass addr direction to `AddWhitelistPermissionFlags. I won't insist though.
💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1884445322)
ACK 6175a2ee09663f7cf20a048b33e537442dfeb81d
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1884445322)
ACK 6175a2ee09663f7cf20a048b33e537442dfeb81d
👍 BrandonOdiwuor approved a pull request: "test: wallet migration, add coverage for tx extra data"
(https://github.com/bitcoin/bitcoin/pull/29204#pullrequestreview-1812762957)
lgtm ACK 016cc807f77a9128d430a0df1edd133521628a33
(https://github.com/bitcoin/bitcoin/pull/29204#pullrequestreview-1812762957)
lgtm ACK 016cc807f77a9128d430a0df1edd133521628a33
💬 darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1884514851)
I think this is due to a missing `std::move` for `j:` which makes `BuildScript` copy `subs[0]`: https://github.com/bitcoin/bitcoin/blob/063a8b83875997068b3eb506b5f30f2691d18052/src/script/miniscript.h#L762C43-L762C43.
Reproducing locally now.
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1884514851)
I think this is due to a missing `std::move` for `j:` which makes `BuildScript` copy `subs[0]`: https://github.com/bitcoin/bitcoin/blob/063a8b83875997068b3eb506b5f30f2691d18052/src/script/miniscript.h#L762C43-L762C43.
Reproducing locally now.
💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1884549947)
Rebased 53f0be73cc816f39e0131f5809bf14b43fee094a -> 673bcf4d25572bc49ff25e06515afe6c2e776537 ([contextSanityChecks_5](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_5) -> [contextSanityChecks_6](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/contextSanityChecks_5..contextSanityChecks_6))
- Fixed conflict with #28051
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1884549947)
Rebased 53f0be73cc816f39e0131f5809bf14b43fee094a -> 673bcf4d25572bc49ff25e06515afe6c2e776537 ([contextSanityChecks_5](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_5) -> [contextSanityChecks_6](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/contextSanityChecks_5..contextSanityChecks_6))
- Fixed conflict with #28051
📝 Satoshin-Btc opened a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/29216)
<!--
*** 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 new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/29216)
<!--
*** 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 new tests that improv
...
✅ fanquake closed a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/29216)
(https://github.com/bitcoin/bitcoin/pull/29216)
📝 fanquake locked a pull request: "Create cmake-multi-platform.yml"
(https://github.com/bitcoin/bitcoin/pull/29216)
<!--
*** 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 new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/29216)
<!--
*** 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 new tests that improv
...
💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447190456)
>Assuming you meant C's feerate is 100 sat/vbyte
Yes C fee rated is 100s/vb, edited thanks.
>This is prior to any relay, of course restricting feerates is incentive compatible once published on the open network
Do you mean incentive incompatible here? because in the given example if C is rejected A will be added to the mempool with mining score of 5s/vb which is lower than whats intended.
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447190456)
>Assuming you meant C's feerate is 100 sat/vbyte
Yes C fee rated is 100s/vb, edited thanks.
>This is prior to any relay, of course restricting feerates is incentive compatible once published on the open network
Do you mean incentive incompatible here? because in the given example if C is rejected A will be added to the mempool with mining score of 5s/vb which is lower than whats intended.
⚠️ ismaelsadeeq opened an issue: "Enable `maxfeerate` and `maxburnamount` as startup config options."
(https://github.com/bitcoin/bitcoin/issues/29217)
### Please describe the feature you'd like to see added.
`maxfeerate` is a sanity check optional parameter used by clients to ensure transactions submitted to the mempool and relayed to the network by a node with`sendrawtransaction`, `testmempoolaccept` and now possibly `submitpackage` #28950 does not exceed the `maxfeerate` value.
Node will reject transactions whose fee rate is higher than `maxfeerate`.
`maxburnamount` is also a sanity check optional parameter used by `sendrawtransaction
...
(https://github.com/bitcoin/bitcoin/issues/29217)
### Please describe the feature you'd like to see added.
`maxfeerate` is a sanity check optional parameter used by clients to ensure transactions submitted to the mempool and relayed to the network by a node with`sendrawtransaction`, `testmempoolaccept` and now possibly `submitpackage` #28950 does not exceed the `maxfeerate` value.
Node will reject transactions whose fee rate is higher than `maxfeerate`.
`maxburnamount` is also a sanity check optional parameter used by `sendrawtransaction
...
💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447202424)
Done in https://github.com/bitcoin/bitcoin/issues/29217
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1447202424)
Done in https://github.com/bitcoin/bitcoin/issues/29217
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884654639)
Pushed up a few more improvements.
Somewhat annoyingly, some distros (Ubuntu) only ship `llvm-libtool-darwin` with a version suffix, i.e `llvm-libtool-darwin-17`, however many others (Fedora, Alpine etc) do not do this. I think we should just set `darwin_LIBTOOL=llvm-libtool-darwin` as a best effort, and I've added a note to the depends readme, that if anyone sees issues, they should set `LIBTOOL=llvm-libtool-darwin-version` when calling make.
The CI failure here is a bit odd, because it
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884654639)
Pushed up a few more improvements.
Somewhat annoyingly, some distros (Ubuntu) only ship `llvm-libtool-darwin` with a version suffix, i.e `llvm-libtool-darwin-17`, however many others (Fedora, Alpine etc) do not do this. I think we should just set `darwin_LIBTOOL=llvm-libtool-darwin` as a best effort, and I've added a note to the depends readme, that if anyone sees issues, they should set `LIBTOOL=llvm-libtool-darwin-version` when calling make.
The CI failure here is a bit odd, because it
...
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1447261373)
I prefer having a single function (as opposed to keeping the logic at the call site) for the following reasons:
* We avoid duplicating code at the RPC call sites:
* The logic for validating SFFO inputs would need to be duplicated at `send`, `fundrawtx` and `walletcreatefundedpsbt`. This is the code that checks that the ints passed are not duplicated and in bounds and previously used to be inside `rpc/FundTransaction`
* We have one place to validate all SFFO inputs:
* Currently, `sendm
...
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1447261373)
I prefer having a single function (as opposed to keeping the logic at the call site) for the following reasons:
* We avoid duplicating code at the RPC call sites:
* The logic for validating SFFO inputs would need to be duplicated at `send`, `fundrawtx` and `walletcreatefundedpsbt`. This is the code that checks that the ints passed are not duplicated and in bounds and previously used to be inside `rpc/FundTransaction`
* We have one place to validate all SFFO inputs:
* Currently, `sendm
...
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884688686)
> The CI failure here is a bit odd, because it only happens for ZeroMQ, and only when using what looks like clang-14?
I think the solution to this is just to keep our current Clang version requirement for macOS cross-compilation. Currently, we vendor our own Clang (17) and use 17 in Guix. Switching to using system Clang doesn't mean we now need to support clang-13 + when building for macOS, because we never did previously, and no-one building Core could have been using a version other than t
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884688686)
> The CI failure here is a bit odd, because it only happens for ZeroMQ, and only when using what looks like clang-14?
I think the solution to this is just to keep our current Clang version requirement for macOS cross-compilation. Currently, we vendor our own Clang (17) and use 17 in Guix. Switching to using system Clang doesn't mean we now need to support clang-13 + when building for macOS, because we never did previously, and no-one building Core could have been using a version other than t
...
💬 theStack commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#issuecomment-1884782328)
Concept ACK
Will review in a bit. As a follow-up idea, maybe we could put the AssumeUTXO chain and snapshot creation in a shared module, in order to deduplicate code between feature_assumeutxo.py and wallet_assumeutxo.py? If we ever wanted to change the snapshot chainstate for e.g. more advanced tests, it'd be quite annoying having to touch several files.
(https://github.com/bitcoin/bitcoin/pull/28838#issuecomment-1884782328)
Concept ACK
Will review in a bit. As a follow-up idea, maybe we could put the AssumeUTXO chain and snapshot creation in a shared module, in order to deduplicate code between feature_assumeutxo.py and wallet_assumeutxo.py? If we ever wanted to change the snapshot chainstate for e.g. more advanced tests, it'd be quite annoying having to touch several files.
💬 brunoerg commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1884820312)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1884820312)
Concept ACK
💬 TheCharlatan commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447366749)
Should be `-17`?
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447366749)
Should be `-17`?
💬 TheCharlatan commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447369323)
Would be good to fix this here imo.
(https://github.com/bitcoin/bitcoin/pull/29205#discussion_r1447369323)
Would be good to fix this here imo.
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447383682)
Thanks, fixed up.
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1447383682)
Thanks, fixed up.
💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884850027)
Given the above, we can also avoid having to repatch Qt, to work around bugs there. Dropped that commit.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1884850027)
Given the above, we can also avoid having to repatch Qt, to work around bugs there. Dropped that commit.