💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757541211)
I think this language is a little bit too strong. It's not risk-free, I think we're just checking that there is no visible incentive for the dust to enter the utxo set.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757541211)
I think this language is a little bit too strong. It's not risk-free, I think we're just checking that there is no visible incentive for the dust to enter the utxo set.
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757548475)
I don't really understand the "or itself being spent by another child, and so on" part
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757548475)
I don't really understand the "or itself being spent by another child, and so on" part
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757536232)
Any reason to use a `CTransaction&` instead of a `CTransactionRef`?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757536232)
Any reason to use a `CTransaction&` instead of a `CTransactionRef`?
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757550476)
?
```suggestion
if (txfee != 0 && std::any_of(tx.vout.cbegin(), tx.vout.cbegin(), [&](const auto& output) { return IsDust(output, dust_relay_fee); })) {
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
}
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757550476)
?
```suggestion
if (txfee != 0 && std::any_of(tx.vout.cbegin(), tx.vout.cbegin(), [&](const auto& output) { return IsDust(output, dust_relay_fee); })) {
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
}
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757552226)
Can you add documentation for when each function must be called, like we did for TRUC?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757552226)
Can you add documentation for when each function must be called, like we did for TRUC?
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757546693)
```suggestion
* Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757546693)
```suggestion
* Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757574587)
This isn't really the desirable behavior, perhaps worth commenting. If we fix this through something like #27476, this should be replaced with a test for the opposite case.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757574587)
This isn't really the desirable behavior, perhaps worth commenting. If we fix this through something like #27476, this should be replaced with a test for the opposite case.
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757578284)
nit: sentence structure kind of weird. the transaction ignores modified? I know what you mean, but maybe confusing wording
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757578284)
nit: sentence structure kind of weird. the transaction ignores modified? I know what you mean, but maybe confusing wording
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757611051)
spend spend
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757611051)
spend spend
💬 furszy commented on issue "Closing a wallet using the fa46088440 28.x QT client segfaults":
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347289507)
The issue is we are calling `removeAndDeleteWallet` twice for the same wallet model. The first time inside `WalletController::closeWallet` and a second time when the backend emits the wallet `unload` signal. Preparing the PR..
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347289507)
The issue is we are calling `removeAndDeleteWallet` twice for the same wallet model. The first time inside `WalletController::closeWallet` and a second time when the backend emits the wallet `unload` signal. Preparing the PR..
👍 jarolrod approved a pull request: "doc: unit test runner help fixup"
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2301431567)
ACK 0024d2c6ea0fafe9b9949af4bbcd0c583e580746
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2301431567)
ACK 0024d2c6ea0fafe9b9949af4bbcd0c583e580746
💬 jarolrod commented on pull request "doc: unit test runner help fixup":
(https://github.com/bitcoin/bitcoin/pull/30890#discussion_r1757593008)
This does change the help to be accurate to what the help command does.
(https://github.com/bitcoin/bitcoin/pull/30890#discussion_r1757593008)
This does change the help to be accurate to what the help command does.
💬 jarolrod commented on pull request "doc: unit test runner help fixup":
(https://github.com/bitcoin/bitcoin/pull/30890#discussion_r1757616784)
agree this is most helpful here
(https://github.com/bitcoin/bitcoin/pull/30890#discussion_r1757616784)
agree this is most helpful here
💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2347290237)
> This one (the third) is very similar to the 'spending' index of electrs (https://github.com/romanz/electrs), only different being how the key is calculated, electrs uses the first 8 bytes of the txid (as uint64) + vout, because the block height or tx position might not be known.
>
> Needing to parse whole blocks to find a transaction for every request impacts lookup time and requires all blocks to be available (so either disallows pruning or needs to request blocks from peers, which hurts p
...
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2347290237)
> This one (the third) is very similar to the 'spending' index of electrs (https://github.com/romanz/electrs), only different being how the key is calculated, electrs uses the first 8 bytes of the txid (as uint64) + vout, because the block height or tx position might not be known.
>
> Needing to parse whole blocks to find a transaction for every request impacts lookup time and requires all blocks to be available (so either disallows pruning or needs to request blocks from peers, which hurts p
...
💬 l0rinc commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2347291739)
I ran benchmarks to evaluate the impact of removing the limit on IBD performance.
The tests were conducted on an Intel Core i7-7700 CPU, 64 GB of RAM, and HDD storage.
I synced up 3 times to block height 600,000 using various `-dbcache` settings: 2 GB, 5 GB, 10 GB, 20 GB, 30 GB, and 40 GB.
<details>
<summary>benchmark</summary>
```bash
hyperfine \
--runs 3 \
--export-json /mnt/ibd_dbcache.json \
--parameter-list DBCACHE 2048,5120,10240,20480,30720,40960 \
--prepare 'rm -rf /mnt/Bit
...
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2347291739)
I ran benchmarks to evaluate the impact of removing the limit on IBD performance.
The tests were conducted on an Intel Core i7-7700 CPU, 64 GB of RAM, and HDD storage.
I synced up 3 times to block height 600,000 using various `-dbcache` settings: 2 GB, 5 GB, 10 GB, 20 GB, 30 GB, and 40 GB.
<details>
<summary>benchmark</summary>
```bash
hyperfine \
--runs 3 \
--export-json /mnt/ibd_dbcache.json \
--parameter-list DBCACHE 2048,5120,10240,20480,30720,40960 \
--prepare 'rm -rf /mnt/Bit
...
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#issuecomment-2347293542)
> ACK [69bf58d](https://github.com/bitcoin/bitcoin/commit/69bf58dc0e25897e9fde435c9823a921590a90dc)
>
> This is definitely an improvement on the existing loadwallet help.
>
> nit: I believe the RPC page listed in the commit message is actually derived from the help output of `loadwallet`. Technically this PR addresses the help output, and downstream things that use it would also benefit. I don't really see a crucial need to change the commit message though, so feel free to ignore. If you h
...
(https://github.com/bitcoin/bitcoin/pull/30302#issuecomment-2347293542)
> ACK [69bf58d](https://github.com/bitcoin/bitcoin/commit/69bf58dc0e25897e9fde435c9823a921590a90dc)
>
> This is definitely an improvement on the existing loadwallet help.
>
> nit: I believe the RPC page listed in the commit message is actually derived from the help output of `loadwallet`. Technically this PR addresses the help output, and downstream things that use it would also benefit. I don't really see a crucial need to change the commit message though, so feel free to ignore. If you h
...
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1757621078)
I have made this last suggested change ^.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1757621078)
I have made this last suggested change ^.
💬 mzumsande commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347297919)
> I believe that's answered positively by the timestamps and structure of the calling code.
not really. I don't see a `"Calling bitcoin-cli: getblocktemplate"` entry in the [log](https://github.com/braidpool/bitcoin/issues/1#issuecomment-2341397203) excerpt, but I do see a `DEBUG result =` log entry.
Both logs are part of `async def bitcoin_cli(self, *args, input=b""):` in your script. That's why I thought the rpc call may have happened before the block was connected by bitcoind.
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347297919)
> I believe that's answered positively by the timestamps and structure of the calling code.
not really. I don't see a `"Calling bitcoin-cli: getblocktemplate"` entry in the [log](https://github.com/braidpool/bitcoin/issues/1#issuecomment-2341397203) excerpt, but I do see a `DEBUG result =` log entry.
Both logs are part of `async def bitcoin_cli(self, *args, input=b""):` in your script. That's why I thought the rpc call may have happened before the block was connected by bitcoind.
📝 fjahr opened a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893)
A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an `ensure` helper to streamline this functionality.
Some approach considerations:
- It is possible to construct this by reusing `wait_until` and wrapping it in `try` internally. However, the logger output of the failing wait would still be printed w
...
(https://github.com/bitcoin/bitcoin/pull/30893)
A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an `ensure` helper to streamline this functionality.
Some approach considerations:
- It is possible to construct this by reusing `wait_until` and wrapping it in `try` internally. However, the logger output of the failing wait would still be printed w
...
💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2347300303)
@davidgumberg Good points. I've made minimal changes in this PR to make sure no position-effecting operations remain, as we're past the feature freeze. Extending the `AutoFile` interface so that `AutoFile::Get` is no longer needed can be done as a follow-up.
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2347300303)
@davidgumberg Good points. I've made minimal changes in this PR to make sure no position-effecting operations remain, as we're past the feature freeze. Extending the `AutoFile` interface so that `AutoFile::Get` is no longer needed can be done as a follow-up.