💬 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.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757628003)
yes, this was my open question https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332066227
Even if we do this change it can still be prioritized for mining *after* entering the mempool; we would need heavier machinery to stop ever treating it as prioritised. This would mean someone would have to create a CPFP transaction then cycle it away, while also using `prioritisetransaction` to get it mined without the dust spent?
@sdaftuar you have any thoughts?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1757628003)
yes, this was my open question https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332066227
Even if we do this change it can still be prioritized for mining *after* entering the mempool; we would need heavier machinery to stop ever treating it as prioritised. This would mean someone would have to create a CPFP transaction then cycle it away, while also using `prioritisetransaction` to get it mined without the dust spent?
@sdaftuar you have any thoughts?
💬 mzumsande commented on pull request "scripted-diff: Modernize nLocalServices naming":
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2347306044)
For the record, I don't like refactor PRs that just rename things to a new convention. It's fine for me if the code is touched anyway, but opening a PR just for that? We probably have hundreds of variables that could be modernized the same way.
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2347306044)
For the record, I don't like refactor PRs that just rename things to a new convention. It's fine for me if the code is touched anyway, but opening a PR just for that? We probably have hundreds of variables that could be modernized the same way.
🤔 jonatack reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2301482520)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2301482520)
Concept ACK
💬 jonatack commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1757631578)
For clarity/readability, suggest using a `timeout=` kwarg in the `ensure` invocations where you pass a timeout value.
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1757631578)
For clarity/readability, suggest using a `timeout=` kwarg in the `ensure` invocations where you pass a timeout value.
💬 fabioBaraDev commented on issue "No such file or directory: bitcoind Error":
(https://github.com/bitcoin/bitcoin/issues/30891#issuecomment-2347309054)
Seems like the bitcoind.cpp file was not compiled and when I try to run the Functional tests, it does not find the folder .... :/
(https://github.com/bitcoin/bitcoin/issues/30891#issuecomment-2347309054)
Seems like the bitcoind.cpp file was not compiled and when I try to run the Functional tests, it does not find the folder .... :/
💬 fjahr commented on pull request "scripted-diff: Modernize nLocalServices naming":
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2347309527)
> For the record, I don't like refactor PRs that just rename things to a new convention. It's fine for me if the code is touched anyway, but opening a PR just for that? We probably have hundreds of variables that could be modernized the same way.
Sure, I agree and I had not opened it if I had not already written the PR and had been encouraged to open it as a follow-up even though the PR where the code was touched was merged without it, see https://github.com/bitcoin/bitcoin/pull/30807#issueco
...
(https://github.com/bitcoin/bitcoin/pull/30885#issuecomment-2347309527)
> For the record, I don't like refactor PRs that just rename things to a new convention. It's fine for me if the code is touched anyway, but opening a PR just for that? We probably have hundreds of variables that could be modernized the same way.
Sure, I agree and I had not opened it if I had not already written the PR and had been encouraged to open it as a follow-up even though the PR where the code was touched was merged without it, see https://github.com/bitcoin/bitcoin/pull/30807#issueco
...