💬 mzumsande commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1844527950)
done (in a slightly adjusted form)
(https://github.com/bitcoin/bitcoin/pull/31223#discussion_r1844527950)
done (in a slightly adjusted form)
💬 ryanofsky commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#issuecomment-2479964213)
Pushed a lot of fixes and updates over the past week and marking this PR as no longer wip/draft. This PR is just a refactoring and doesn't change runtime behavior, but it should be a good start to having better defined settings with clear types and default values, avoiding confusion and bugs caused by the current settings API seen in #30529 and #31212 and other PRs, and making API more extensible to support custom types and validation in the future.
(https://github.com/bitcoin/bitcoin/pull/31260#issuecomment-2479964213)
Pushed a lot of fixes and updates over the past week and marking this PR as no longer wip/draft. This PR is just a refactoring and doesn't change runtime behavior, but it should be a good start to having better defined settings with clear types and default values, avoiding confusion and bugs caused by the current settings API seen in #30529 and #31212 and other PRs, and making API more extensible to support custom types and validation in the future.
💬 mzumsande commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2479974458)
Updated Release note and added a commit that adds a functional test for various aspects of `-port` that have been discussed in this PR an the issue.
When it comes to the question whether the issue should be fixed in 28.1, I guess it also comes down to how widespread we think a scenario with `-port` and a manual `torrc` is, vs how many more people doing local testing setups would be affected by the `-port` change from 28.0.
I can't say I have a good feeling for either - to be honest I would h
...
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2479974458)
Updated Release note and added a commit that adds a functional test for various aspects of `-port` that have been discussed in this PR an the issue.
When it comes to the question whether the issue should be fixed in 28.1, I guess it also comes down to how widespread we think a scenario with `-port` and a manual `torrc` is, vs how many more people doing local testing setups would be affected by the `-port` change from 28.0.
I can't say I have a good feeling for either - to be honest I would h
...
⚠️ techy2 opened an issue: "depends build fails - libevent not in CMakeLists"
(https://github.com/bitcoin/bitcoin/issues/31299)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
CMake Error: The source directory "/master/gitrepo/bitcoin/depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.12-stable-9d8e3ddea5c/build" does not appear to contain CMakeLists.txt.
### Expected behaviour
expect it to build
### Steps to reproduce
git clone https://github.com/bitcoin/bitcoin
cd bitcoin
git checkout v28.0
cd depends
make
### Relevant log output
/master/gitrepo
...
(https://github.com/bitcoin/bitcoin/issues/31299)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
CMake Error: The source directory "/master/gitrepo/bitcoin/depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.12-stable-9d8e3ddea5c/build" does not appear to contain CMakeLists.txt.
### Expected behaviour
expect it to build
### Steps to reproduce
git clone https://github.com/bitcoin/bitcoin
cd bitcoin
git checkout v28.0
cd depends
make
### Relevant log output
/master/gitrepo
...
💬 techy2 commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2479997928)
that's 128 G ram
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2479997928)
that's 128 G ram
📝 adamandrews1 converted_to_draft a pull request: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298)
Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.
fixes #25980
(https://github.com/bitcoin/bitcoin/pull/31298)
Previously, combinerawtransaction would silently return the first tx when asked to combine unrelated txs. Now, it will check tx mergeability and throws a descriptive error if tx cannot be merged.
fixes #25980
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844630125)
Using `importscript`, you can import arbitrary scripts that can appear as output scripts. It is thus possible to import a P2WSH script which contains the hash of a P2SH script or a witness program. The user can then use `importscripts` again to import those particular scripts.
Most of the edge cases revolve around the user doing something insane, but because they were allowed, we have to handle them.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844630125)
Using `importscript`, you can import arbitrary scripts that can appear as output scripts. It is thus possible to import a P2WSH script which contains the hash of a P2SH script or a witness program. The user can then use `importscripts` again to import those particular scripts.
Most of the edge cases revolve around the user doing something insane, but because they were allowed, we have to handle them.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844642514)
**sol**ution**s**
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844642514)
**sol**ution**s**
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844644393)
Generally "keys" refers to private keys and public keys are typically specified as such or as "pubkey".
In this instance, multisigs are only spendable if we have all of their private keys, regardless of the context the multisig appears in.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844644393)
Generally "keys" refers to private keys and public keys are typically specified as such or as "pubkey".
In this instance, multisigs are only spendable if we have all of their private keys, regardless of the context the multisig appears in.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844645672)
All of the above. All scripts are stored in mapScripts, without contexts. Thus all scripts in mapScripts can appear in any context. That is the insanity of legacy IsMine.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844645672)
All of the above. All scripts are stored in mapScripts, without contexts. Thus all scripts in mapScripts can appear in any context. That is the insanity of legacy IsMine.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844650818)
If a P2PK or P2PKH script exists in mapScripts, then the corresponding P2SH-P2PK and P2SH-P2PKH output script is spendable.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844650818)
If a P2PK or P2PKH script exists in mapScripts, then the corresponding P2SH-P2PK and P2SH-P2PKH output script is spendable.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844651416)
Output script level.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844651416)
Output script level.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844652679)
It's basically just casting types.
`sols[0]` contains a vector of bytes which represents the hash of a pubkey. This is cast to the `uint160` type, which is then cast to the `CKeyID` type (which is really just a wrapper around `uint160`) which is the type that `GetPubKey` can take. There is no direct conversion from `std::vector<>` to `CKeyID` hence the `uint160` in between.
It doesn't make sense to pull this apart since the intermediate types are never used.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844652679)
It's basically just casting types.
`sols[0]` contains a vector of bytes which represents the hash of a pubkey. This is cast to the `uint160` type, which is then cast to the `CKeyID` type (which is really just a wrapper around `uint160`) which is the type that `GetPubKey` can take. There is no direct conversion from `std::vector<>` to `CKeyID` hence the `uint160` in between.
It doesn't make sense to pull this apart since the intermediate types are never used.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844723731)
It is possible that the solver could be rewritten to pull out all of the components of scripts into structs with proper typing and member names, but that is a much larger refactor that is out of scope for this PR.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844723731)
It is possible that the solver could be rewritten to pull out all of the components of scripts into structs with proper typing and member names, but that is a much larger refactor that is out of scope for this PR.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844850200)
Done
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844850200)
Done
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844850300)
I've added a comment.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844850300)
I've added a comment.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844850356)
I've added additional tests to `wallet_migration.py`.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844850356)
I've added additional tests to `wallet_migration.py`.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851400)
This comment has been reworded
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851400)
This comment has been reworded
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851480)
I've added some additional comments.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851480)
I've added some additional comments.
💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851563)
I've reworded this comment.
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851563)
I've reworded this comment.