💬 fanquake commented on pull request "test: Remove unused sanitizer suppressions":
(https://github.com/bitcoin/bitcoin/pull/27498#issuecomment-1516418849)
Might as well have a go

📝 glozow opened a pull request: "mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501)
Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up `mapDeltas` manually. When `CTxMemPool::PrioritiseTransaction` sets a delta to 0, remove the entry from `mapDeltas`.

Motivation / Background
- `mapDeltas` entries are never removed from mapDeltas except when the tx is mined in a block or conflicted.
- Mostly it is a feature to allow `prioritisetransaction` for a tx that isn't in the mempool {yet, anymore}. A user can may
...

💬 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
...

💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(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

👍 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`

💬 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?

💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(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.

💬 sipa commented on pull request "BIP324: ElligatorSwift integrations":
(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`.

💬 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.

💬 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.

💬 instagibbs commented on pull request "BIP324: ElligatorSwift integrations":
(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)`

💬 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>?

💬 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?

💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(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
...

💬 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