💬 Daniel600 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794185915)
> I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
At GAP600
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794185915)
> I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
At GAP600
...
💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794248918)
Re https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1669359482
> What are the steps to reproduce?
Run the fuzzer on the fuzz test introduced in this PR seeded with Sjors' inputs from his [qa repo PR](https://github.com/bitcoin-core/qa-assets/pull/140) and compiled with bdb enabled.
> Outside of that, my recommendations would be to use bdb-5.3
The bugs persist even with bdb 5.3.
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794248918)
Re https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1669359482
> What are the steps to reproduce?
Run the fuzzer on the fuzz test introduced in this PR seeded with Sjors' inputs from his [qa repo PR](https://github.com/bitcoin-core/qa-assets/pull/140) and compiled with bdb enabled.
> Outside of that, my recommendations would be to use bdb-5.3
The bugs persist even with bdb 5.3.
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794251948)
The first commit ee79f05f1fef57bfbea24333a46e525385d02319 introduces a dangling reference warning for me. gcc 13.1.0 on Ubuntu 23.04.
```
./configure --enable-suppress-external-warnings --disable-fuzz-binary --without-gui
make
...
validation.cpp: In member function ‘PackageMempoolAcceptResult {anonymous}::MemPoolAccept::AcceptPackage(const Package&, ATMPArgs&)’:
validation.cpp:1488:36: warning: possibly dangling reference to a temporary [-Wdangling-reference]
1488 | const
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794251948)
The first commit ee79f05f1fef57bfbea24333a46e525385d02319 introduces a dangling reference warning for me. gcc 13.1.0 on Ubuntu 23.04.
```
./configure --enable-suppress-external-warnings --disable-fuzz-binary --without-gui
make
...
validation.cpp: In member function ‘PackageMempoolAcceptResult {anonymous}::MemPoolAccept::AcceptPackage(const Package&, ATMPArgs&)’:
validation.cpp:1488:36: warning: possibly dangling reference to a temporary [-Wdangling-reference]
1488 | const
...
💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794275787)
> > I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
>
> HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
>
> At G
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794275787)
> > I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
>
> HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
>
> At G
...
💬 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.