💬 achow101 commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055281)
Done
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1938055281)
Done
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938059816)
These checks all occur inside of `migrate_and_get_rpc` now so they're redundant.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938059816)
These checks all occur inside of `migrate_and_get_rpc` now so they're redundant.
💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938074664)
The verb "fill" suggests to me that the caller is filling the descriptor with a private key, rather than the descriptor filling something else. I think "Get" is still appropriate.
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938074664)
The verb "fill" suggests to me that the caller is filling the descriptor with a private key, rather than the descriptor filling something else. I think "Get" is still appropriate.
💬 achow101 commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938075462)
Valid means that the origin is correct. It may not be useful, e.g. the origin of a random key is itself, but it's valid and correct.
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938075462)
Valid means that the origin is correct. It may not be useful, e.g. the origin of a random key is itself, but it's valid and correct.
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938079255)
This PR is big enough already, I'd prefer to do that separately.
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938079255)
This PR is big enough already, I'd prefer to do that separately.
💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628555787)
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.
Maybe I am still not getting it but re-reading the release notes once more, I don't think they reflect this correctly. They seem to suggest that setting `-blockmaxweight` to `4,000,000` does change something. I think they need to explain what is actually happening so that miners can make the right decision. Worst case (though maybe a bit u
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2628555787)
> No, the current behavior on master is that we always reserve 8,000 WU, regardless of what blockmaxweight is. This is actually how I discovered the issue.
Maybe I am still not getting it but re-reading the release notes once more, I don't think they reflect this correctly. They seem to suggest that setting `-blockmaxweight` to `4,000,000` does change something. I think they need to explain what is actually happening so that miners can make the right decision. Worst case (though maybe a bit u
...
💬 achow101 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938086462)
No... that's not at all how deserialization works. There's no constructors called, `prev_txid` is an already initialized `uint256`. Unserializing into it means that the data is overwritten. There's no `std::make_shared` called anywhere, nothing is constructed or initialized.
The call chain is `UnserializeFromVector` -> `UnserializeMany` -> `uint256::Unserialize()` -> `Unserialize(Stream& s, Span<B> span)` -> `<template Stream> read()`, and every `read(Span)` function checks that the stream ha
...
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1938086462)
No... that's not at all how deserialization works. There's no constructors called, `prev_txid` is an already initialized `uint256`. Unserializing into it means that the data is overwritten. There's no `std::make_shared` called anywhere, nothing is constructed or initialized.
The call chain is `UnserializeFromVector` -> `UnserializeMany` -> `uint256::Unserialize()` -> `Unserialize(Stream& s, Span<B> span)` -> `<template Stream> read()`, and every `read(Span)` function checks that the stream ha
...
🤔 mzumsande reviewed a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382#pullrequestreview-2587949312)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31382#pullrequestreview-2587949312)
Concept ACK
💬 mzumsande commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1938025806)
Not too familiar with the shutdown sequence - I don't fully understand why we need to flush the chainstates twice.
Do you know any examples of events that result in the second flush on destruction doing any actual work in bitcoind?
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1938025806)
Not too familiar with the shutdown sequence - I don't fully understand why we need to flush the chainstates twice.
Do you know any examples of events that result in the second flush on destruction doing any actual work in bitcoind?
📝 tiagosh opened a pull request: "rpc: collect transaction fees on generateblock"
(https://github.com/bitcoin/bitcoin/pull/31775)
Fixes #31684
Currently `generateblock` will insert the specified transactions directly into the new block without collecting fees in the coinbase transaction.
This patch changes the code to take transaction fees into account and make it behave closer to the expected miner behavior in mainnet.
(https://github.com/bitcoin/bitcoin/pull/31775)
Fixes #31684
Currently `generateblock` will insert the specified transactions directly into the new block without collecting fees in the coinbase transaction.
This patch changes the code to take transaction fees into account and make it behave closer to the expected miner behavior in mainnet.
💬 Tiffanywaters92 commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1938118477)
So can I ask the worst question lol what is it that's wrong or needed with bitcoin with all of this? Simplest like for dummies kinda answer
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1938118477)
So can I ask the worst question lol what is it that's wrong or needed with bitcoin with all of this? Simplest like for dummies kinda answer
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2628621716)
Marking this as ready for review as it no longer has any prerequisites.
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2628621716)
Marking this as ready for review as it no longer has any prerequisites.
👋 achow101's pull request is ready for review: "wallet: Disable creating and loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/31250)
(https://github.com/bitcoin/bitcoin/pull/31250)
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121255)
I think it's possible, but I can't figure it out right now.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121255)
I think it's possible, but I can't figure it out right now.
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121303)
Done
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121303)
Done
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121319)
Done
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121319)
Done
💬 achow101 commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121540)
I changed it to `Assume` and added an early return here so that the failed migration is cleaned up.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1938121540)
I changed it to `Assume` and added an early return here so that the failed migration is cleaned up.
💬 hebasto commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2628639378)
> But I am surprised to see clang-tidy build succeeding.
So am I.
> There were so much other clang-tidy output in the previous failing job aside from the missing input file errors fixed by the cmake change: https://cirrus-ci.com/task/4607289014878208?logs=ci#L7174. But I guess all the other new clang-tidy output was warnings not errors, so they don't cause the job to fail.
It could be the result of the absence of `WarningsAsErrors: '*'` in `src/ipc/libmultiprocess/.clang-tidy`.
> Sti
...
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2628639378)
> But I am surprised to see clang-tidy build succeeding.
So am I.
> There were so much other clang-tidy output in the previous failing job aside from the missing input file errors fixed by the cmake change: https://cirrus-ci.com/task/4607289014878208?logs=ci#L7174. But I guess all the other new clang-tidy output was warnings not errors, so they don't cause the job to fail.
It could be the result of the absence of `WarningsAsErrors: '*'` in `src/ipc/libmultiprocess/.clang-tidy`.
> Sti
...
💬 brunoerg commented on pull request "rpc: collect transaction fees on generateblock":
(https://github.com/bitcoin/bitcoin/pull/31775#discussion_r1938131196)
```
19:24:04.713] /ci_container_base/src/node/miner.cpp:150:9: error: 'exclusive_locks_required' attribute cannot be applied to a statement
[19:24:04.713] 150 | EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs);
[19:24:04.713] | ^ ~
[19:24:04.713] /ci_container_base/src/threadsafety.h:31:54: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED'
[19:24:04.713] 31 | #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_requir
...
(https://github.com/bitcoin/bitcoin/pull/31775#discussion_r1938131196)
```
19:24:04.713] /ci_container_base/src/node/miner.cpp:150:9: error: 'exclusive_locks_required' attribute cannot be applied to a statement
[19:24:04.713] 150 | EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs);
[19:24:04.713] | ^ ~
[19:24:04.713] /ci_container_base/src/threadsafety.h:31:54: note: expanded from macro 'EXCLUSIVE_LOCKS_REQUIRED'
[19:24:04.713] 31 | #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_requir
...
📝 tdb3 converted_to_draft a pull request: "rpc: add `relevant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713)
Currently, after starting a scan with `scanblocks` the user must wait until the scan is complete to see the associated/relevant block hashes.
This updates the `scanblocks status` result object to provide the `relevant_blocks` found so far.
This enables earlier subsequent lookup within matching blocks (e.g. to run `getdescriptoractivity` in PR #30708)
Below is an example tested on signet by starting and obtaining status of a scan for an address that has received many coinbase outputs (so the
...
(https://github.com/bitcoin/bitcoin/pull/30713)
Currently, after starting a scan with `scanblocks` the user must wait until the scan is complete to see the associated/relevant block hashes.
This updates the `scanblocks status` result object to provide the `relevant_blocks` found so far.
This enables earlier subsequent lookup within matching blocks (e.g. to run `getdescriptoractivity` in PR #30708)
Below is an example tested on signet by starting and obtaining status of a scan for an address that has received many coinbase outputs (so the
...