💬 lontivero commented on issue "Improving fee estimation accuracy":
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-1611989703)
Given the current fee rate estimation algorithm has no prediction power when the mempool conditions change, it is common to pay too little or too much, what is especially true while we increase the confirmation target.
In Wasabi we use the feerate histogram to "correct" the fee rate estimations provided by our node and to keep them between an acceptable range. There are many things that can be done but we just want to be sure our users participating in coinjoins don't pay more than every
...
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-1611989703)
Given the current fee rate estimation algorithm has no prediction power when the mempool conditions change, it is common to pay too little or too much, what is especially true while we increase the confirmation target.
In Wasabi we use the feerate histogram to "correct" the fee rate estimations provided by our node and to keep them between an acceptable range. There are many things that can be done but we just want to be sure our users participating in coinjoins don't pay more than every
...
💬 jonatack commented on pull request "refactor: Drop unsafe AsBytePtr function":
(https://github.com/bitcoin/bitcoin/pull/27978#issuecomment-1612028668)
re-ACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9
The OP and commit message might need to be updated a bit.
(https://github.com/bitcoin/bitcoin/pull/27978#issuecomment-1612028668)
re-ACK 7c853619ee9ea17a79f1234b6c7871a45ceadcb9
The OP and commit message might need to be updated a bit.
💬 achow101 commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1612048492)
ACK d4fb58ae8ae9772d025ead184ef8f2c0ea50df3e
Much simpler to understand, and the more complicated stuff matches up with the descriptions in WIkipedia.
(https://github.com/bitcoin/bitcoin/pull/26222#issuecomment-1612048492)
ACK d4fb58ae8ae9772d025ead184ef8f2c0ea50df3e
Much simpler to understand, and the more complicated stuff matches up with the descriptions in WIkipedia.
🚀 achow101 merged a pull request: "Introduce secp256k1 module with field and group classes to test framework"
(https://github.com/bitcoin/bitcoin/pull/26222)
(https://github.com/bitcoin/bitcoin/pull/26222)
💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1612067010)
> > Note that this affects only our own self-advertisements, the rules to forward other people's addrs are not changed.
>
> Would this make our own addr stand out too? Or could we still forward our own addr by coincidence outside of self-advertising?
Yes, we could still forward it: When we receive a gossipped address we choose 1-2 random peers (that stay the same for 24 hours, see [RelayAddress](https://github.com/bitcoin/bitcoin/blob/7952a5934a1e071ca24a51483d058174e7b6de43/src/net_proces
...
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1612067010)
> > Note that this affects only our own self-advertisements, the rules to forward other people's addrs are not changed.
>
> Would this make our own addr stand out too? Or could we still forward our own addr by coincidence outside of self-advertising?
Yes, we could still forward it: When we receive a gossipped address we choose 1-2 random peers (that stay the same for 24 hours, see [RelayAddress](https://github.com/bitcoin/bitcoin/blob/7952a5934a1e071ca24a51483d058174e7b6de43/src/net_proces
...
🤔 jonatack reviewed a pull request: "test: Use same timeout for all index sync"
(https://github.com/bitcoin/bitcoin/pull/27988#pullrequestreview-1503945589)
Approach ACK
(review beg for #27425 doing a ~similar test util extraction change)
(https://github.com/bitcoin/bitcoin/pull/27988#pullrequestreview-1503945589)
Approach ACK
(review beg for #27425 doing a ~similar test util extraction change)
💬 jonatack commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#discussion_r1245722202)
Perhaps call it `WaitUntilIndexSynced`
and `s/Block/Wait/`, or drop the Doxygen comment if the naming is clear enough
(https://github.com/bitcoin/bitcoin/pull/27988#discussion_r1245722202)
Perhaps call it `WaitUntilIndexSynced`
and `s/Block/Wait/`, or drop the Doxygen comment if the naming is clear enough
💬 jonatack commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#discussion_r1245731192)
Here and in the function declaration
```suggestion
void IndexWaitSynced(const BaseIndex& index)
```
(https://github.com/bitcoin/bitcoin/pull/27988#discussion_r1245731192)
Here and in the function declaration
```suggestion
void IndexWaitSynced(const BaseIndex& index)
```
💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245800503)
Cool thanks!. Pushed it with few tiny differences because I think that, at least for now, we don't need the indirections introduced by making `GetFirstStoredBlock` a `FindFirstStored` wrapper.
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245800503)
Cool thanks!. Pushed it with few tiny differences because I think that, at least for now, we don't need the indirections introduced by making `GetFirstStoredBlock` a `FindFirstStored` wrapper.
🤔 ismaelsadeeq reviewed a pull request: "assumeutxo (2)"
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1504152824)
While testing.
>Once you get the full headers chain, you'll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk
`FlushSnapshotToDisk: saving snapshot chain state (10588 MB) started` takes about 5 hours to complete on pop-OS 22.
During the `FlushSnapshotToDisk` process, I believe there should be some progress indication, similar to what occurs during the snapshot loading process. Currently, it appears to be like it is stuck.
Shutting down `bit
...
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1504152824)
While testing.
>Once you get the full headers chain, you'll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk
`FlushSnapshotToDisk: saving snapshot chain state (10588 MB) started` takes about 5 hours to complete on pop-OS 22.
During the `FlushSnapshotToDisk` process, I believe there should be some progress indication, similar to what occurs during the snapshot loading process. Currently, it appears to be like it is stuck.
Shutting down `bit
...
🤔 jonatack reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1504151388)
Initial concept/approach ACK 500e31e87b33418a0747a0f3118db9ceb23896be
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1504151388)
Initial concept/approach ACK 500e31e87b33418a0747a0f3118db9ceb23896be
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1245804422)
In 930b1c7323be84ad7fb8371ee744f201325f18bb agree with @MarcoFalke to put this next to `nTime`, and it may be nice to document how each is used and why the need for both
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1245804422)
In 930b1c7323be84ad7fb8371ee744f201325f18bb agree with @MarcoFalke to put this next to `nTime`, and it may be nice to document how each is used and why the need for both
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1245801404)
In e16fb7d212bf1dbb5086ed8f452e8e80acbe8325 consider adding the `info_for_relay()` getter declaration next to `info()` and `infoAll()`
<details><summary>diff</summary><p>
```diff
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -411,8 +411,6 @@ public:
using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
std::vector<std::pair<uint256, txiter>> vTxHashes GUARDED_BY(cs); //!< All tx witness hashes/entries in mapTx, in random order
- TxMempoolInfo in
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1245801404)
In e16fb7d212bf1dbb5086ed8f452e8e80acbe8325 consider adding the `info_for_relay()` getter declaration next to `info()` and `infoAll()`
<details><summary>diff</summary><p>
```diff
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -411,8 +411,6 @@ public:
using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
std::vector<std::pair<uint256, txiter>> vTxHashes GUARDED_BY(cs); //!< All tx witness hashes/entries in mapTx, in random order
- TxMempoolInfo in
...
💬 mzumsande commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1612168354)
> Looks like there is a variation with ERROR: Commit: Failed to commit latest basic block filter index state?
>And the same without the error:
It's different tests for different indexes: `txindex_tests/txindex_initial_sync` fails without the Commit Error, `blockfilter_index_tests/blockfilter_index_initial_sync` fails with it (but same root cause obviously).
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1612168354)
> Looks like there is a variation with ERROR: Commit: Failed to commit latest basic block filter index state?
>And the same without the error:
It's different tests for different indexes: `txindex_tests/txindex_initial_sync` fails without the Commit Error, `blockfilter_index_tests/blockfilter_index_initial_sync` fails with it (but same root cause obviously).
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1612198266)
Found an issue with prefix cursor handling, updated the tests to check for that. Also pulled in @TheCharlatan's fuzz test and changed several asserts to instead throw exceptions which resolves the fuzz crash.
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1612198266)
Found an issue with prefix cursor handling, updated the tests to check for that. Also pulled in @TheCharlatan's fuzz test and changed several asserts to instead throw exceptions which resolves the fuzz crash.
💬 MarcusMWilliams commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1612199588)
Marcus Maurice Williams
On Mon, May 8, 2023, 10:12 AM jamesob ***@***.***> wrote:
>
> - Background and FAQ:
> https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
> - Prior progress/project:
> https://github.com/bitcoin/bitcoin/projects/11
> - Replaces #15606 <https://github.com/bitcoin/bitcoin/pull/15606>,
> which was closed due to Github slowness. Original description and
> commentary can be found there.
>
> -----------------------------
...
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1612199588)
Marcus Maurice Williams
On Mon, May 8, 2023, 10:12 AM jamesob ***@***.***> wrote:
>
> - Background and FAQ:
> https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
> - Prior progress/project:
> https://github.com/bitcoin/bitcoin/projects/11
> - Replaces #15606 <https://github.com/bitcoin/bitcoin/pull/15606>,
> which was closed due to Github slowness. Original description and
> commentary can be found there.
>
> -----------------------------
...
💬 LarryRuane commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-1612227295)
Rebased to fix merge conflict
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-1612227295)
Rebased to fix merge conflict
💬 Trrvrr commented on pull request "[24.1] Final Changes":
(https://github.com/bitcoin/bitcoin/pull/27660#issuecomment-1612298568)
> Final changes for `v24.1`.
>
> PR for bitcoincore.org is here: https://github.com/bitcoin-core/bitcoincore.org/pull/968.
(https://github.com/bitcoin/bitcoin/pull/27660#issuecomment-1612298568)
> Final changes for `v24.1`.
>
> PR for bitcoincore.org is here: https://github.com/bitcoin-core/bitcoincore.org/pull/968.
💬 craigraw commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1612482503)
To be clear, my desire for an efficient way to retrieve fee rate histogram data has nothing to do with Bitcoin Core's fee estimation algorithm. Because fee estimation is inherently difficult and no algorithm can know the user's time preference for every transaction, Sparrow Wallet displays the fee rate histogram chart in the UI as another "tool in the toolbox". This allows a user to observe trends and adjust the fee rate to place a transaction in a particular fee rate band to suit their time pre
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1612482503)
To be clear, my desire for an efficient way to retrieve fee rate histogram data has nothing to do with Bitcoin Core's fee estimation algorithm. Because fee estimation is inherently difficult and no algorithm can know the user's time preference for every transaction, Sparrow Wallet displays the fee rate histogram chart in the UI as another "tool in the toolbox". This allows a user to observe trends and adjust the fee rate to place a transaction in a particular fee rate band to suit their time pre
...
💬 MarcoFalke commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1612498668)
> One workaround is to use `getrawmempool false` followed by repeated calls to `getmempoolentry`. This of course is even more inefficient, leading any reasonable client implementation to maintain a replica of mempool entries to avoid having to load them all, one by one, every time a fee rate histogram is required. This trades better performance for significantly increased memory utilisation.
I think the `getmempoolentry` can be batched for a slight improvement. Also, you don't have to store t
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1612498668)
> One workaround is to use `getrawmempool false` followed by repeated calls to `getmempoolentry`. This of course is even more inefficient, leading any reasonable client implementation to maintain a replica of mempool entries to avoid having to load them all, one by one, every time a fee rate histogram is required. This trades better performance for significantly increased memory utilisation.
I think the `getmempoolentry` can be batched for a slight improvement. Also, you don't have to store t
...