💬 glozow commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1516503220)
**Alternatives Considered**
Feel free to suggest more alternatives, but note potential problems.
(1) Call TrimToSize() at smart intervals, e.g. when a timestamp changes (since packages are submitted with the same timestamp), or every ~25 transactions, or when we hit a certain threshold above maxmempool.
(2) When a transaction fails for fee-related reasons, try again with bypass_limits=true, and then schedule a flush for later.
(3) Create a new mempool.dat format, which contains a list of
...
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1516503220)
**Alternatives Considered**
Feel free to suggest more alternatives, but note potential problems.
(1) Call TrimToSize() at smart intervals, e.g. when a timestamp changes (since packages are submitted with the same timestamp), or every ~25 transactions, or when we hit a certain threshold above maxmempool.
(2) When a transaction fails for fee-related reasons, try again with bypass_limits=true, and then schedule a flush for later.
(3) Create a new mempool.dat format, which contains a list of
...
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739143)
Done.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739143)
Done.
💬 MarcoFalke commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172733241)
```suggestion
"Returns a map of all fee deltas in memory pool by txid, and whether the tx is present in mempool.",
```
nit
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172733241)
```suggestion
"Returns a map of all fee deltas in memory pool by txid, and whether the tx is present in mempool.",
```
nit
👍 MarcoFalke approved a pull request: "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1394217946)
Concept ACK. Wanted to add a `deletepriority` RPC myself, but I guess it is easy to implement a for loop in python utilising the output of `getprioritisationmap`
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1394217946)
Concept ACK. Wanted to add a `deletepriority` RPC myself, but I guess it is easy to implement a for loop in python utilising the output of `getprioritisationmap`
💬 MarcoFalke commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172737216)
Won't this need mempool.cs to ensure an atomic result? Otherwise you may get nonsense back on a data race, like a parent missing from the mempool, when the child is in the mempool, no?
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172737216)
Won't this need mempool.cs to ensure an atomic result? Otherwise you may get nonsense back on a data race, like a parent missing from the mempool, when the child is in the mempool, no?
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739437)
Added some comments. Please have a look.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739437)
Added some comments. Please have a look.
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739546)
Done.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739546)
Done.
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739678)
Done.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739678)
Done.
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739938)
Done. Also renamed to just `SIZE`.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172739938)
Done. Also renamed to just `SIZE`.
💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172741182)
Your math is correct. And indeed, it's such a low probability event that it's not worth adding code to deal with it (even outside of a benchmark, I'd say). Still, I've added an assert to make it clear to the reader this is always supposed to be the case.
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172741182)
Your math is correct. And indeed, it's such a low probability event that it's not worth adding code to deal with it (even outside of a benchmark, I'd say). Still, I've added an assert to make it clear to the reader this is always supposed to be the case.
💬 glozow commented on pull request "mempool: keep CPFP'd transactions when loading from mempool.dat":
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1516508803)
> Maybe move most of the text in the PR description into a comment? "Review this first" and "Alternatives considered" stuff is useful here, but doesn't seem useful for the merge commit message?
Moved "alternatives considered." I prefer putting "review this first" at the top so reviewers see it as soon as possible. It will go away if/when I rebase, so definitely before any merging happens.
(https://github.com/bitcoin/bitcoin/pull/27476#issuecomment-1516508803)
> Maybe move most of the text in the PR description into a comment? "Review this first" and "Alternatives considered" stuff is useful here, but doesn't seem useful for the merge commit message?
Moved "alternatives considered." I prefer putting "review this first" at the top so reviewers see it as soon as possible. It will go away if/when I rebase, so definitely before any merging happens.
💬 instagibbs commented on pull request "BIP324: ElligatorSwift integrations":
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172742351)
significantly clearer, thanks
(https://github.com/bitcoin/bitcoin/pull/27479#discussion_r1172742351)
significantly clearer, thanks
💬 glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172747649)
Yes good point. Will add a lock-already-held `exists` and put this all under `LOCK(mempool.cs)`
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172747649)
Yes good point. Will add a lock-already-held `exists` and put this all under `LOCK(mempool.cs)`
💬 glozow commented on pull request "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172748986)
Oh, or would it be better to have `CTxMemPool::GetPrioritisationMap` take the lock and create a map from txid to pair<fee, bool>?
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1172748986)
Oh, or would it be better to have `CTxMemPool::GetPrioritisationMap` take the lock and create a map from txid to pair<fee, bool>?
💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1172760920)
Correct. Do you want me to add it to the doc?
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1172760920)
Correct. Do you want me to add it to the doc?
💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1172763708)
Nice, pushed your test
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1172763708)
Nice, pushed your test
🤔 stickies-v reviewed a pull request: "blockstorage: do not flush block to disk if it is already there"
(https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1289406384)
Approach ACK 93c70287a6434c6c665a211dc4dfbbd9c3db4083 (modulo tests)
Conceptually, this seems straightforward: we don't want to update block files during reindex, so avoiding the flushing altogether is desireable.
I think the only potential for unexpected behaviour change is if there are places other than `BlockManager::SaveBlockToDisk()` where block files are modified (and potentially not flushed). Looking at the callsites of `FlatFileSeq::Open()` and all of its parent callsites, it seems
...
(https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1289406384)
Approach ACK 93c70287a6434c6c665a211dc4dfbbd9c3db4083 (modulo tests)
Conceptually, this seems straightforward: we don't want to update block files during reindex, so avoiding the flushing altogether is desireable.
I think the only potential for unexpected behaviour change is if there are places other than `BlockManager::SaveBlockToDisk()` where block files are modified (and potentially not flushed). Looking at the callsites of `FlatFileSeq::Open()` and all of its parent callsites, it seems
...
💬 stickies-v commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1162797652)
nit: some inconsistent usage of `BOOST_CHECK` and `BOOST_CHECK_EQUAL`, I think using `BOOST_CHECK_EQUAL` here and in a few other places makes sense
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1162797652)
nit: some inconsistent usage of `BOOST_CHECK` and `BOOST_CHECK_EQUAL`, I think using `BOOST_CHECK_EQUAL` here and in a few other places makes sense
💬 stickies-v commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1171578190)
nit: unintuitive name, it's not a block
```suggestion
FlatFilePos last_block_pos;
```
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1171578190)
nit: unintuitive name, it's not a block
```suggestion
FlatFilePos last_block_pos;
```
💬 stickies-v commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1164077010)
Is this a reliable way to check for a FAT filesystem? WIN32 could also use other filesystems I think (e.g. NTFS), and likewise - FAT could still be used without WIN32, I think?
I think I agree with [LarryRuane](https://github.com/bitcoin/bitcoin/pull/27039/files#r1122400769) that comparing files is probably more robust and portable, which I think is reasonable with these small file sizes? But perhaps you've found this is too much of a performance hit?
You could also check the filesystem di
...
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1164077010)
Is this a reliable way to check for a FAT filesystem? WIN32 could also use other filesystems I think (e.g. NTFS), and likewise - FAT could still be used without WIN32, I think?
I think I agree with [LarryRuane](https://github.com/bitcoin/bitcoin/pull/27039/files#r1122400769) that comparing files is probably more robust and portable, which I think is reasonable with these small file sizes? But perhaps you've found this is too much of a performance hit?
You could also check the filesystem di
...