💬 theuni commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895813848)
Concept ACK. Anything to clean this up :)
See also #32317 which moves some of this functionality around similarly.
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895813848)
Concept ACK. Anything to clean this up :)
See also #32317 which moves some of this functionality around similarly.
💬 darosior commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-2895815514)
Can someone with permissions turn this into draft?
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-2895815514)
Can someone with permissions turn this into draft?
✅ fjahr closed a pull request: "RFC: Accept non-std transactions in Testnet4 by default again"
(https://github.com/bitcoin/bitcoin/pull/32133)
(https://github.com/bitcoin/bitcoin/pull/32133)
💬 fjahr commented on pull request "RFC: Accept non-std transactions in Testnet4 by default again":
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2895823345)
> Only after you've signed the transaction in your test environment with a real mainnet key that's protecting real mainnet funds.
I think it's reasonable for a bitcoin company to have a few sats in a mainnet hot wallet to use for testing, they don't need to touch their cold storage for this.
Anyway, closing for now, my opinion on this hasn't changed much but I think it's better to focus on something else.
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2895823345)
> Only after you've signed the transaction in your test environment with a real mainnet key that's protecting real mainnet funds.
I think it's reasonable for a bitcoin company to have a few sats in a mainnet hot wallet to use for testing, they don't need to touch their cold storage for this.
Anyway, closing for now, my opinion on this hasn't changed much but I think it's better to focus on something else.
💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2098877081)
Not really, seems like it was changed in that specific file and resolved without being changed more broadly.
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2098877081)
Not really, seems like it was changed in that specific file and resolved without being changed more broadly.
💬 achow101 commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2895845004)
> `createrawtransaction` doesn't do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.
`fundrawtransaction` does though, and that was my point. This PR doesn't touch `fundrawtransaction` (or rather the wallet's coin selection code) and hence this is incomplete.
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2895845004)
> `createrawtransaction` doesn't do other policy checks either, such as OP_RETURN checks, so one could argue this is fine as well.
`fundrawtransaction` does though, and that was my point. This PR doesn't touch `fundrawtransaction` (or rather the wallet's coin selection code) and hence this is incomplete.
🤔 mzumsande reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2855643647)
Code Review ACK 2b202e9db56487e658fc41089178f31ef4259a0d
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2855643647)
Code Review ACK 2b202e9db56487e658fc41089178f31ef4259a0d
👋 fjahr's pull request is ready for review: "RFC: refactor: Split multithreaded case out of CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/32575)
(https://github.com/bitcoin/bitcoin/pull/32575)
💬 fjahr commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895912036)
Cleaned up the comments so this should be ready for more detailed feedback, but expect a rebase once https://github.com/bitcoin/bitcoin/pull/32467 is merged.
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895912036)
Cleaned up the comments so this should be ready for more detailed feedback, but expect a rebase once https://github.com/bitcoin/bitcoin/pull/32467 is merged.
💬 fjahr commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2098936599)
I have been contemplating kicking this out (here and in `CheckInputScripts`) or even turning this into an assert because the callers already handle this everywhere. On the other hand it conceptually makes sense to return early if we know there is nothing to check because we don't expect any inputs. So I am still undecided on this one.
(https://github.com/bitcoin/bitcoin/pull/32575#discussion_r2098936599)
I have been contemplating kicking this out (here and in `CheckInputScripts`) or even turning this into an assert because the callers already handle this everywhere. On the other hand it conceptually makes sense to return early if we know there is nothing to check because we don't expect any inputs. So I am still undecided on this one.
💬 TheCharlatan commented on pull request "RFC: refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895950866)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2895950866)
Concept ACK
💬 davidgumberg commented on pull request "qt: drop unused watch-only functionality":
(https://github.com/bitcoin/bitcoin/pull/32459#issuecomment-2895951950)
Review ACK https://github.com/bitcoin/bitcoin/commit/e8661aac752eb08fee318eb8f56e599578d78f9f.
Quickly tested with GUI built on master and GUI built on this branch. Confirmed the watch-only behavior removed here has nothing to do with no-private-keys descriptor wallets, this is dead code AFAICT.
(https://github.com/bitcoin/bitcoin/pull/32459#issuecomment-2895951950)
Review ACK https://github.com/bitcoin/bitcoin/commit/e8661aac752eb08fee318eb8f56e599578d78f9f.
Quickly tested with GUI built on master and GUI built on this branch. Confirmed the watch-only behavior removed here has nothing to do with no-private-keys descriptor wallets, this is dead code AFAICT.
💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2896320121)
re: https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2895479357
> I found that `bitcoin-node` will (still) segfault if i try to stop it while the client is running.
Thanks for testing this! I created a new issue in https://github.com/bitcoin-core/libmultiprocess/issues/176 to help debug. So far I couldn't reproduce the problem running a very basic client there, but I can try to extend it to do more of the operations your client is doing and see if I can make the problem happen. I
...
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2896320121)
re: https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-2895479357
> I found that `bitcoin-node` will (still) segfault if i try to stop it while the client is running.
Thanks for testing this! I created a new issue in https://github.com/bitcoin-core/libmultiprocess/issues/176 to help debug. So far I couldn't reproduce the problem running a very basic client there, but I can try to extend it to do more of the operations your client is doing and see if I can make the problem happen. I
...
🤔 maflcko reviewed a pull request: "Fees: add Fee rate Forecaster Manager"
(https://github.com/bitcoin/bitcoin/pull/31664#pullrequestreview-2856553062)
(from the llm linter)
(https://github.com/bitcoin/bitcoin/pull/31664#pullrequestreview-2856553062)
(from the llm linter)
💬 maflcko commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099527580)
unable a fee rate -> unable to find a fee rate [Incorrect grammar; "unable to find" or "unable to provide" would be clearer.]
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099527580)
unable a fee rate -> unable to find a fee rate [Incorrect grammar; "unable to find" or "unable to provide" would be clearer.]
💬 maflcko commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099526827)
could remove the redundant comments, given that at least one of them is wrong?
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2099526827)
could remove the redundant comments, given that at least one of them is wrong?
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific targets":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2896845391)
concept ack, but yeah, needs rebase
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-2896845391)
concept ack, but yeah, needs rebase
💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2896961033)
utACK b23cefb4f3abe33c7bc933b60f1c0d15137c627c
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2896961033)
utACK b23cefb4f3abe33c7bc933b60f1c0d15137c627c
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099616827)
Sounds fine, forgot to push this?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099616827)
Sounds fine, forgot to push this?
💬 maflcko commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2896998818)
> I wonder if there is a platform without `posix_fallocate` (outside of macos and windows). If not, the fallback can probably be fully removed, along with the detection and it can just be assumed to be always present?
I am asking, because the fallback was just the normal code in the beginning (bba89aa82a80f0373dcb7288d96d5b0fcb453d73) and was not removed in 288fdc092aff9d7e0cea159196b2e96044a786c7.
However, even bioniC has `posix_fallocate` since API level 21 (https://android.googlesource
...
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2896998818)
> I wonder if there is a platform without `posix_fallocate` (outside of macos and windows). If not, the fallback can probably be fully removed, along with the detection and it can just be assumed to be always present?
I am asking, because the fallback was just the normal code in the beginning (bba89aa82a80f0373dcb7288d96d5b0fcb453d73) and was not removed in 288fdc092aff9d7e0cea159196b2e96044a786c7.
However, even bioniC has `posix_fallocate` since API level 21 (https://android.googlesource
...