📝 sipa opened a pull request: "Modernize rpcauth.py"
(https://github.com/bitcoin/bitcoin/pull/27081)
Use Python3 constructions, and f-strings.
(https://github.com/bitcoin/bitcoin/pull/27081)
Use Python3 constructions, and f-strings.
💬 mzumsande commented on pull request "test: Fix intermittent sync issue in wallet_pruning":
(https://github.com/bitcoin/bitcoin/pull/27066#issuecomment-1426452302)
https://cirrus-ci.com/task/5441089956478976 :cry:
(https://github.com/bitcoin/bitcoin/pull/27066#issuecomment-1426452302)
https://cirrus-ci.com/task/5441089956478976 :cry:
💬 ariard commented on pull request "consensus: Remove mainnet checkpoints":
(https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103347369)
Yeah, no strong opinion here. Doesn't seem the most complex commit to reverse if needed. At the same time good to cleanup the code (or add a historical note to explain what where checkpoints and why unused code is still there i don't know).
(https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103347369)
Yeah, no strong opinion here. Doesn't seem the most complex commit to reverse if needed. At the same time good to cleanup the code (or add a historical note to explain what where checkpoints and why unused code is still there i don't know).
💬 dergoegge commented on pull request "mempool: Increase mempool default size":
(https://github.com/bitcoin/bitcoin/pull/27079#issuecomment-1426479599)
Concept NACK🪄
The rational that is provided here is unconvincing.
(https://github.com/bitcoin/bitcoin/pull/27079#issuecomment-1426479599)
Concept NACK🪄
The rational that is provided here is unconvincing.
💬 achow101 commented on pull request "wallet: reuse change dest when re-creating TX with avoidpartialspends":
(https://github.com/bitcoin/bitcoin/pull/27053#discussion_r1103462065)
I think this is actually more than a nit, it can have consequences on the change position of the change output for the aps solution.
If the user specified a change position but we didn't use it, now `change_pos` would be `-1` , so the aps run would put its change in a random position, if it needs one. This could put the change in an unexpected position. So we shouldn't overwrite nor shadow `change_pos`.
Good catch!
(https://github.com/bitcoin/bitcoin/pull/27053#discussion_r1103462065)
I think this is actually more than a nit, it can have consequences on the change position of the change output for the aps solution.
If the user specified a change position but we didn't use it, now `change_pos` would be `-1` , so the aps run would put its change in a random position, if it needs one. This could put the change in an unexpected position. So we shouldn't overwrite nor shadow `change_pos`.
Good catch!
💬 achow101 commented on pull request "wallet: rpc to add automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/25907#issuecomment-1426565205)
Per the discussion during the IRC meeting on 2023-01-27, I've removed the ability to rotate the active hd key. `sethdseed` can be used to set one on a blank wallet, but if there is already on set, then it cannot be changed.
(https://github.com/bitcoin/bitcoin/pull/25907#issuecomment-1426565205)
Per the discussion during the IRC meeting on 2023-01-27, I've removed the ability to rotate the active hd key. `sethdseed` can be used to set one on a blank wallet, but if there is already on set, then it cannot be changed.
💬 ishaanam commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1099038619)
In ee620e999a8ce269a90eee15b25988428e7620fd " tests: Tests for migrating wallets by name, and providing passphrase "
It would be useful to also test the behavior of the wallet encryption post-migration (eg. that `migratewallet` re-locks the wallet). A test for when the passphrase is provided but is incorrect could also be added here. A test for when the wallet name is provided but is incorrect could also be added.
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1099038619)
In ee620e999a8ce269a90eee15b25988428e7620fd " tests: Tests for migrating wallets by name, and providing passphrase "
It would be useful to also test the behavior of the wallet encryption post-migration (eg. that `migratewallet` re-locks the wallet). A test for when the passphrase is provided but is incorrect could also be added here. A test for when the wallet name is provided but is incorrect could also be added.
💬 ishaanam commented on pull request "wallet: be able to specify a wallet name and passphrase to migratewallet":
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1103485473)
nit:
```suggestion
// If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it
```
(https://github.com/bitcoin/bitcoin/pull/26595#discussion_r1103485473)
nit:
```suggestion
// If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it
```
💬 davidgumberg commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1426612195)
ACK https://github.com/bitcoin/bitcoin/commit/d98c39a1274bfcab3935ffdb75165abf85073e88
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1426612195)
ACK https://github.com/bitcoin/bitcoin/commit/d98c39a1274bfcab3935ffdb75165abf85073e88
💬 codo1 commented on pull request "Modernize rpcauth.py":
(https://github.com/bitcoin/bitcoin/pull/27081#issuecomment-1426626622)
tACK
Tested as follows: generated new userpw using an explicit password. Used it on signet through an RPC request (checked I got 'incorrect password attempt' if I changed the userpw a bit).
(https://github.com/bitcoin/bitcoin/pull/27081#issuecomment-1426626622)
tACK
Tested as follows: generated new userpw using an explicit password. Used it on signet through an RPC request (checked I got 'incorrect password attempt' if I changed the userpw a bit).
📝 TysonsTech opened a pull request: "Minor edits - punctuation, spelling to make the contributing instructions more readable for all. "
(https://github.com/bitcoin/bitcoin/pull/27082)
Greetings - Wanted to ease into my first Bitcoin core contribution with some text updates. Small changes but I think the doc reads a bit better. Cheers!
(https://github.com/bitcoin/bitcoin/pull/27082)
Greetings - Wanted to ease into my first Bitcoin core contribution with some text updates. Small changes but I think the doc reads a bit better. Cheers!
💬 1440000bytes commented on pull request "mempool: Increase mempool default size":
(https://github.com/bitcoin/bitcoin/pull/27079#issuecomment-1426640141)

(https://github.com/bitcoin/bitcoin/pull/27079#issuecomment-1426640141)

📝 hebasto opened a pull request: "ci: Improve `ccache` cache hit rates"
(https://github.com/bitcoin/bitcoin/pull/27083)
Changes that are not affecting binaries, e.g., #27070 or #27072, are expected to show up to 100% `ccache` hit rate. But that is not the case:
- https://cirrus-ci.com/build/4720733276864512
- https://cirrus-ci.com/build/5915469295648768
Zero hit rate in the "macOS 10.15" is well [known](https://github.com/bitcoin/bitcoin/issues/21552). But a [fix](https://github.com/bitcoin/bitcoin/pull/24620) was [considered](https://github.com/bitcoin/bitcoin/pull/24620#issuecomment-1100850691) not "a good
...
(https://github.com/bitcoin/bitcoin/pull/27083)
Changes that are not affecting binaries, e.g., #27070 or #27072, are expected to show up to 100% `ccache` hit rate. But that is not the case:
- https://cirrus-ci.com/build/4720733276864512
- https://cirrus-ci.com/build/5915469295648768
Zero hit rate in the "macOS 10.15" is well [known](https://github.com/bitcoin/bitcoin/issues/21552). But a [fix](https://github.com/bitcoin/bitcoin/pull/24620) was [considered](https://github.com/bitcoin/bitcoin/pull/24620#issuecomment-1100850691) not "a good
...
💬 hebasto commented on pull request "ci: Improve `ccache` cache hit rates":
(https://github.com/bitcoin/bitcoin/pull/27083#issuecomment-1426664401)
Drafted for now as I'm going to demonstrate benefits of this PR by re-running CI twice. First time with cleaned caches, and second one to ensure higher hit rates.
(https://github.com/bitcoin/bitcoin/pull/27083#issuecomment-1426664401)
Drafted for now as I'm going to demonstrate benefits of this PR by re-running CI twice. First time with cleaned caches, and second one to ensure higher hit rates.
💬 Sjors commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1426676349)
One thing we could do to mitigate the concern of missing witness data, is to download it for a small random sample of blocks. When that fails, we fall back to redownloading _all_ witness data (which would presumably also fail). So in a scenario where witness data is really gone, pruned nodes will behave the same as unpruned nodes during IBD.
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1426676349)
One thing we could do to mitigate the concern of missing witness data, is to download it for a small random sample of blocks. When that fails, we fall back to redownloading _all_ witness data (which would presumably also fail). So in a scenario where witness data is really gone, pruned nodes will behave the same as unpruned nodes during IBD.
💬 Sjors commented on pull request "consensus: Remove mainnet checkpoints":
(https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103582363)
I think it's fine to drop the checkpoints in one release and then drop the checkpoint logic in a later release.
(https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103582363)
I think it's fine to drop the checkpoints in one release and then drop the checkpoint logic in a later release.
💬 bipulkmr-crypto commented on issue "Check usages of `#if defined(...)`":
(https://github.com/bitcoin/bitcoin/issues/16419#issuecomment-1426679579)
Is this issue up for grab?
(https://github.com/bitcoin/bitcoin/issues/16419#issuecomment-1426679579)
Is this issue up for grab?
💬 Sjors commented on pull request "test: p2p: check that headers message with invalid proof-of-work disconnects peer":
(https://github.com/bitcoin/bitcoin/pull/26184#issuecomment-1426685146)
ACK 772671245d50d94fd5087deb2542854604eba174
(https://github.com/bitcoin/bitcoin/pull/26184#issuecomment-1426685146)
ACK 772671245d50d94fd5087deb2542854604eba174
📝 hebasto converted_to_draft a pull request: "ci: Make `ccache` hashing content of compiler binary"
(https://github.com/bitcoin/bitcoin/pull/27077)
By default, `ccache` includes the modification time (`mtime`) and size of the compiler in the hash to ensure that results retrieved from the cache are accurate. But in CI environment compiler's `mtime` can be unique each run.
Effectively, this PR fixes https://github.com/bitcoin/bitcoin/pull/27063#issuecomment-1423901418:
> Looks like ccache isn't working here
>
> https://cirrus-ci.com/task/4571293245243392?logs=ci#L4178
(https://github.com/bitcoin/bitcoin/pull/27077)
By default, `ccache` includes the modification time (`mtime`) and size of the compiler in the hash to ensure that results retrieved from the cache are accurate. But in CI environment compiler's `mtime` can be unique each run.
Effectively, this PR fixes https://github.com/bitcoin/bitcoin/pull/27063#issuecomment-1423901418:
> Looks like ccache isn't working here
>
> https://cirrus-ci.com/task/4571293245243392?logs=ci#L4178
👋 hebasto's pull request is ready for review: "ci: Make `ccache` hashing content of compiler binary"
(https://github.com/bitcoin/bitcoin/pull/27077)
(https://github.com/bitcoin/bitcoin/pull/27077)
💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1426688833)
We should add a warning in the release notes that, although you can safely downgrade the node, you should _not_ encrypt the wallet while using that older version. And add a test that shows what happens.
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1426688833)
We should add a warning in the release notes that, although you can safely downgrade the node, you should _not_ encrypt the wallet while using that older version. And add a test that shows what happens.