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

💬 ajtowns commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1220348795)
Could add a `-deprecatedrpc=prioritisetransactionresult` flag to allow access to the current format of result.

💬 ajtowns commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1220330124)
This is `"transactionid"` in `getrawmempool` and `getmempoolancestors`, probably better to be consistent?

💬 ajtowns commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1220349890)
Probably better to either expose it via the rpc, or not have the useless code at all?

💬 ajtowns commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1220335611)
Provides a good excuse to map to an object rather than just a the delta itself, which in turn might make it easier to add other info in future without having a breaking change?

💬 ajtowns commented on pull request "doc: Clarify -datacarriersize, add -datacarriersize=2 tests":
(https://github.com/bitcoin/bitcoin/pull/27832#issuecomment-1579448029)
Concept ACK

💬 instagibbs commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1220360880)
100% for object for future-proofing, either way

💬 theStack commented on pull request "Introduce 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1579485693)
FWIW, I was curious how hard it would be to implement `getblockfileinfo` in the functional test framework by parsing the block files: https://github.com/theStack/bitcoin/blob/97ea647ee9b186ff4d3be2ba5ac91b94e5371323/test/functional/test_framework/util.py#L559-L584 (branch https://github.com/theStack/bitcoin/tree/getblockfileinfo_python)

The result seems to work, but is still kind of ugly and hacky. To get a parsed block's height (without flooding the node with hundreds of `getblockheader` RPC
...

💬 theStack commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1579532617)
> > 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
...

💬 hebasto commented on pull request "guix: Clean up manifest":
(https://github.com/bitcoin/bitcoin/pull/27811#issuecomment-1579928821)
My Guix build:
```
c4f2c29c7b5e1ab211ea38358517cf6c2a1e154e5d2c0d036e7750a8f5f5555a guix-build-77414d50d6f7/output/aarch64-linux-gnu/SHA256SUMS.part
c902944cce64fe7f2cee1242f18b35dbe1c5ce515e921c08c9bfebb34d0c28ca guix-build-77414d50d6f7/output/aarch64-linux-gnu/bitcoin-77414d50d6f7-aarch64-linux-gnu-debug.tar.gz
3aad68ed102f2f1a18217b3d2b2b32ba51f266c03b971f05f8bebd160bbf7d75 guix-build-77414d50d6f7/output/aarch64-linux-gnu/bitcoin-77414d50d6f7-aarch64-linux-gnu.tar.gz
36737484c80cdfc10
...

👋 MarcoFalke's pull request is ready for review: "p2p: Stop relaying non-mempool txs"
(https://github.com/bitcoin/bitcoin/pull/27625)

💬 joostjager commented on pull request "[mempool] allow tx replacement by smaller witness":
(https://github.com/bitcoin/bitcoin/pull/24007#issuecomment-1579986129)
Regarding use cases: perhaps the potential coinjoin problem with annexes mentioned in https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-June/021736.html can be mitigated with this PR.

💬 achow101 commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#issuecomment-1580072467)
Silent merge conflict (with `-werror`)

```
../../../src/test/miniminer_tests.cpp: In member function ‘void miniminer_tests::miniminer_1p1c::test_method()’:
../../../src/test/miniminer_tests.cpp:145:66: error: narrowing conversion of ‘((const boost::operators_impl::dereferenceable<boost::multi_index::detail::hashed_index_iterator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detai
...

👋 achow101's pull request is ready for review: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914)