💬 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
...
💬 maflcko commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1757633150)
```suggestion
def ensure(self, test_function, *, duration):
```
I think calling this "timeout" may be a bit confusing, because it will always "timeout". Maybe "duration" can be used to communicate that this always takes a fixed span of time?
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1757633150)
```suggestion
def ensure(self, test_function, *, duration):
```
I think calling this "timeout" may be a bit confusing, because it will always "timeout". Maybe "duration" can be used to communicate that this always takes a fixed span of time?
💬 maflcko commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1757636065)
Why should this be scaled by the timeout factor? If someone increased that sufficiently to not have to deal with timeouts, they will be bitten by tests that never terminate. IIRC the CI has a large factor, so it may lead to timeouts there.
I think just making the duration parameter a required named arg would be cleaner and also better document that the test will spend that amount of time "sleeping".
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1757636065)
Why should this be scaled by the timeout factor? If someone increased that sufficiently to not have to deal with timeouts, they will be bitten by tests that never terminate. IIRC the CI has a large factor, so it may lead to timeouts there.
I think just making the duration parameter a required named arg would be cleaner and also better document that the test will spend that amount of time "sleeping".
💬 maflcko commented on issue "No such file or directory: bitcoind Error":
(https://github.com/bitcoin/bitcoin/issues/30891#issuecomment-2347317546)
Please share the configure summary, the make output, and the exact commands.
(https://github.com/bitcoin/bitcoin/issues/30891#issuecomment-2347317546)
Please share the configure summary, the make output, and the exact commands.
📝 furszy opened a pull request: "gui: fix crash when closing wallet"
(https://github.com/bitcoin-core/gui/pull/835)
The crash occurs because `WalletController::removeAndDeleteWallet` is called twice for the
same wallet model: first in the GUI's button connected function `WalletController::closeWallet`,
and then again when the backend emits the `WalletModel::unload` signal.
This causes the issue because `removeAndDeleteWallet` inlines an `erase(std::remove())`.
So, if `std::remove` returns an iterator to the end (indicating the element wasn't found
because it was already erased), the subsequent call to
...
(https://github.com/bitcoin-core/gui/pull/835)
The crash occurs because `WalletController::removeAndDeleteWallet` is called twice for the
same wallet model: first in the GUI's button connected function `WalletController::closeWallet`,
and then again when the backend emits the `WalletModel::unload` signal.
This causes the issue because `removeAndDeleteWallet` inlines an `erase(std::remove())`.
So, if `std::remove` returns an iterator to the end (indicating the element wasn't found
because it was already erased), the subsequent call to
...
💬 furszy commented on issue "Closing a wallet using the fa46088440 28.x QT client segfaults":
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347322116)
See https://github.com/bitcoin-core/gui/pull/835
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347322116)
See https://github.com/bitcoin-core/gui/pull/835