👍 instagibbs approved a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1465157911)
code review ACK 6d36f3edbe7549cfaadc48b43c09dd3e581e92ff

I'd really prefer we have a regtest-only option to skip the check to impact integration tests the least.

💬 instagibbs commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1578737483)
> Yea exactly but that requires the user to track the txid and its delta or after https://github.com/bitcoin/bitcoin/pull/27501 is merged track the txid call getpriotisationmap and then use the negative delta from there
> was thinking it would be easier if the user could just specify the txid they want to remove

I tend to think that 27501 is enough? prioritization is a very advanced feature in itself, seems to be sufficient to me.

💬 instagibbs commented on pull request "test: Add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#issuecomment-1578765163)
ACK https://github.com/bitcoin/bitcoin/pull/27832/commits/fa4f476d517669d42342223b5f03fb2025a12a74

👍 instagibbs approved a pull request: "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1465223614)
code review ACK 67b7fecacd0489809690982c89ba2d0acdca938c

just test suggestions, everything looks good

💬 instagibbs commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219665212)
is there a particular reason we're exposing in-mempool-ness here? No strong feelings, just curious.

💬 instagibbs commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219668280)
just directly construct the entire 2-entry map and assert here? means we wouldn't miss extraneous entries in future regression. Could also keep a running "tally" object that's modified along with the test cases

💬 instagibbs commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219669511)
same as above?

💬 instagibbs commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219669146)
same as above?

💬 instagibbs commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219680499)
maybe add a test case that shows prioritising by value of `0` doesn't add an entry, to cover all the bases

💬 kevkevinpal commented on issue "depriortisetransaction":
(https://github.com/bitcoin/bitcoin/issues/27807#issuecomment-1578836291)
> > Yea exactly but that requires the user to track the txid and its delta or after #27501 is merged track the txid call getpriotisationmap and then use the negative delta from there
> > was thinking it would be easier if the user could just specify the txid they want to remove
>
> I tend to think that 27501 is enough? prioritization is a very advanced feature in itself, seems to be sufficient to me.

ok yea makes sense and doesn't seem like anyone strongly wants this feature either, so I
...

✅ kevkevinpal closed an issue: "depriortisetransaction"
(https://github.com/bitcoin/bitcoin/issues/27807)

💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1578839709)


On June 6, 2023 1:36:09 AM GMT+02:00, Antoine Riard ***@***.***> wrote:
>Note for the clarity, I think effectively both a loosening (e.g nVersion=3 where TX_MAX_STANDARD_VERSION allows nVersion=3 transaction propagation) and a tightening (e.g some nSequence field usage being banned from propagation) can be used to commit a double-spend.
>
>With a loosening, if the victim full-node is not upgraded, they won't accept your new nVersion=3 transactions, and you can feed the rest of the upgraded net
...

💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1578839879)


On June 6, 2023 3:32:01 AM GMT+02:00, Anthony Towns ***@***.***> wrote:
>> Note for the clarity, I think effectively both a loosening (e.g nVersion=3 where TX_MAX_STANDARD_VERSION allows nVersion=3 transaction propagation) and a tightening (e.g some nSequence field usage being banned from propagation) can be used to commit a double-spend.
>
>Any mempool acceptance policy difference between two nodes can be used to prevent your node from seeing transactions the other node accepts. If you outrig
...

💬 miketwenty1 commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#discussion_r1219839773)
> Do you mean to say that the user can combine data and address entries?

The outputs array, `outputs []`, must contain at minimum either an obj containing `{"address" ".."}` or an obj containing `{"data": ".."}`. You can also specify both, but you must have at least one.

I'm reading over this block of help text again.
```
(json array, required) The outputs (key-value pairs), where none of the keys are duplicated.
That is, each address can only app
...

🤔 theStack reviewed a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1465636707)
Concept ACK

Regarding the conclusion in the first commit (_"'StartShutdown' should only be used for user requested shutdowns. Internal errors that cause a shutdown should use 'AbortNode'."_), the following is probably another candidate where `AbortNode` is preferred over `StartShutdown`?

https://github.com/bitcoin/bitcoin/blob/8cc65f093c0cf7a27b3bfc6da90fd7a6ac8f5e48/src/index/base.cpp#L33-L41

(not saying it necessarily has to happen in this PR though, if it makes sense).

🤔 furszy reviewed a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1465970269)
> Regarding the conclusion in the first commit (_"'StartShutdown' should only be used for user requested shutdowns. Internal errors that cause a shutdown should use 'AbortNode'."_), the following is probably another candidate where `AbortNode` is preferred over `StartShutdown`?

Yeah absolutely. Good eye. If you compare it in-detail, that function is a plain copy of `AbortNode`: it (1) calls `SetMiscWarning`, (2) logs the error, (3) notifies the UI by calling `InitError` (noui listeners as wel
...

💬 ajtowns commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1579369447)
ACK 355bc6098a7373feb1d59f9651a79e1477d22243

🤔 ajtowns reviewed a pull request: "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1466050778)
ACK 67b7fecacd0489809690982c89ba2d0acdca938c code review only, some nits

💬 ajtowns commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1220320231)
Should just be `""` rather than `"prioritisation-map"` ?

💬 ajtowns commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1220309713)
Seems strange to make these `const`. `const auto deltas = mempool.GetPrioritisedTransactions()` should be enough...