💬 S3RK commented on pull request "wallet: cache descriptor ID to avoid repeated descriptor string creation":
(https://github.com/bitcoin/bitcoin/pull/28799#issuecomment-1794276769)
Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
The change looks correct. I also verified that when we update descriptors with `importdescriptor` command we replace the whole `WalletDescriptor` object, so ID remains correct in that case.
(https://github.com/bitcoin/bitcoin/pull/28799#issuecomment-1794276769)
Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
The change looks correct. I also verified that when we update descriptors with `importdescriptor` command we replace the whole `WalletDescriptor` object, so ID remains correct in that case.
📝 ajtowns opened a pull request: "ArgsManager: support subcommand-specific options"
(https://github.com/bitcoin/bitcoin/pull/28802)
Adds the ability to link particular options to one or more subcommands, and uses this feature in `bitcoin-wallet`. Separates out the help information for subcommand-specific options (duplicating it if an option applies to multiple subcommands), and provides a function for checking if some options have been specified that only apply to different subcommands.
(https://github.com/bitcoin/bitcoin/pull/28802)
Adds the ability to link particular options to one or more subcommands, and uses this feature in `bitcoin-wallet`. Separates out the help information for subcommand-specific options (duplicating it if an option applies to multiple subcommands), and provides a function for checking if some options have been specified that only apply to different subcommands.
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-1794358855)
Implements [this comment](https://github.com/bitcoin/bitcoin/pull/20715#discussion_r555574662) from when these commands were introduced originally.
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-1794358855)
Implements [this comment](https://github.com/bitcoin/bitcoin/pull/20715#discussion_r555574662) from when these commands were introduced originally.
💬 jsarenik commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1794367265)
@ajtowns wrote:
> What's the actual problem you're trying to solve here? Would it work to have the signet miner only generate blocks when the mempool has entries, and sit idle when it's empty, for instance? If you're trying to generate many blocks, then regtest is already better as it doesn't require a baseline proof of work, and simply having multiple regtest configs probably makes more sense.
The last [comment](https://pricey.pages.dev/loglog) makes a point. It really is hard when anyone c
...
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1794367265)
@ajtowns wrote:
> What's the actual problem you're trying to solve here? Would it work to have the signet miner only generate blocks when the mempool has entries, and sit idle when it's empty, for instance? If you're trying to generate many blocks, then regtest is already better as it doesn't require a baseline proof of work, and simply having multiple regtest configs probably makes more sense.
The last [comment](https://pricey.pages.dev/loglog) makes a point. It really is hard when anyone c
...
💬 maflcko commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794381260)
Ok, but then I think this is a problem for the qa-assets repo to solve. As I said, BDB is not enabled in the fuzz CI, so this shouldn't cause CI issues.
One way for qa-assets to solve this would be to exclude inputs that crash bdb.
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794381260)
Ok, but then I think this is a problem for the qa-assets repo to solve. As I said, BDB is not enabled in the fuzz CI, so this shouldn't cause CI issues.
One way for qa-assets to solve this would be to exclude inputs that crash bdb.
💬 maflcko commented on issue "test: wallet_miniscript.py fails with thread sanitizer ":
(https://github.com/bitcoin/bitcoin/issues/28800#issuecomment-1794386419)
This also happens on the qemu-s390x and valgrind CI tasks
(https://github.com/bitcoin/bitcoin/issues/28800#issuecomment-1794386419)
This also happens on the qemu-s390x and valgrind CI tasks
💬 maflcko commented on pull request "build: Drop no longer needed MSVC warning suppressions":
(https://github.com/bitcoin/bitcoin/pull/28798#issuecomment-1794394106)
lgtm ACK 33223f9d550b66885a15e2fe45147d629cc359d2
(https://github.com/bitcoin/bitcoin/pull/28798#issuecomment-1794394106)
lgtm ACK 33223f9d550b66885a15e2fe45147d629cc359d2
✅ maflcko closed a pull request: "RPC/Wallet: Access wallets via interfaces::Wallet"
(https://github.com/bitcoin/bitcoin/pull/26082)
(https://github.com/bitcoin/bitcoin/pull/26082)
💬 maflcko commented on pull request "RPC/Wallet: Access wallets via interfaces::Wallet":
(https://github.com/bitcoin/bitcoin/pull/26082#issuecomment-1794410487)
Closing for now. Please leave a comment here, to have it re-opened by someone.
(https://github.com/bitcoin/bitcoin/pull/26082#issuecomment-1794410487)
Closing for now. Please leave a comment here, to have it re-opened by someone.
🤔 Sjors reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1714571006)
Concept ACK. Lightly reviewed the refactoring (as of a20c1d2b81a702f97267d4f098f13ed80e9320cf) which all looks reasonable, but I'm not very familiar with mempool code.
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1714571006)
Concept ACK. Lightly reviewed the refactoring (as of a20c1d2b81a702f97267d4f098f13ed80e9320cf) which all looks reasonable, but I'm not very familiar with mempool code.
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382967058)
ee79f05f1fef57bfbea24333a46e525385d02319: `{`
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382967058)
ee79f05f1fef57bfbea24333a46e525385d02319: `{`
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382997719)
6b6793e6835dd2ddf367490362042bf34eb260a3: The only caller `checkChainLimits` of is the wallet's `CreateTransactionInternal`, which will say "Transaction has too long of a mempool chain" regardless of which limit is hit.
A (wallet) followup should propagate `err_string` up.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1382997719)
6b6793e6835dd2ddf367490362042bf34eb260a3: The only caller `checkChainLimits` of is the wallet's `CreateTransactionInternal`, which will say "Transaction has too long of a mempool chain" regardless of which limit is hit.
A (wallet) followup should propagate `err_string` up.
💬 maflcko commented on pull request "Fix typos and suggest doing so before branch-off":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794414689)
Are you still working on this? Otherwise, I think it can be closed, so that it can be picked up by someone else, if it is still relevant.
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794414689)
Are you still working on this? Otherwise, I think it can be closed, so that it can be picked up by someone else, if it is still relevant.
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383019317)
ee79f05f1fef57bfbea24333a46e525385d02319: this fixes the dangling warning:
```cpp
const CTxMemPoolEntry& entry{Assert(m_pool.GetEntry(txid)).value()};
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383019317)
ee79f05f1fef57bfbea24333a46e525385d02319: this fixes the dangling warning:
```cpp
const CTxMemPoolEntry& entry{Assert(m_pool.GetEntry(txid)).value()};
```
💬 Sjors commented on pull request "Fix typos and suggest doing so before branch-off":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794431063)
I was still waiting for feedback from maintainers, but I'll just drop the last commit.
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794431063)
I was still waiting for feedback from maintainers, but I'll just drop the last commit.
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383022517)
Thank, might be a good idea to bracket initialize in other places too.
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1383022517)
Thank, might be a good idea to bracket initialize in other places too.
💬 willcl-ark commented on pull request "doc: Add offline signing tutorial":
(https://github.com/bitcoin/bitcoin/pull/28363#issuecomment-1794436797)
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
Looks good to me!
(https://github.com/bitcoin/bitcoin/pull/28363#issuecomment-1794436797)
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
Looks good to me!
💬 dergoegge commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1794443638)
Here's a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28578/fuzz.coverage/index.html (~1000h CPU time).
Is there a reason not to cover more of `DescriptorScriptPubKeyMan` in this PR? e.g. `DescriptorScriptPubKeyMan::SignTransaction` or `DescriptorScriptPubKeyMan::FillPSBT`.
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1794443638)
Here's a coverage report: https://dergoegge.github.io/bitcoin-coverage/pr28578/fuzz.coverage/index.html (~1000h CPU time).
Is there a reason not to cover more of `DescriptorScriptPubKeyMan` in this PR? e.g. `DescriptorScriptPubKeyMan::SignTransaction` or `DescriptorScriptPubKeyMan::FillPSBT`.
🚀 fanquake merged a pull request: "build: Drop no longer needed MSVC warning suppressions"
(https://github.com/bitcoin/bitcoin/pull/28798)
(https://github.com/bitcoin/bitcoin/pull/28798)
💬 fanquake commented on pull request "Fix typos":
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794459605)
```bash
test/functional/feature_assumeutxo.py:78: refering ==> referring
test/functional/feature_assumeutxo.py:115: refering ==> referring
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
(https://github.com/bitcoin/bitcoin/pull/28605#issuecomment-1794459605)
```bash
test/functional/feature_assumeutxo.py:78: refering ==> referring
test/functional/feature_assumeutxo.py:115: refering ==> referring
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```