💬 hebasto commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677477117)
Concept ACK.

💬 MarcoFalke commented on pull request "ci: Use hard-coded root path for CI containers (bugfix)":
(https://github.com/bitcoin/bitcoin/pull/28185#issuecomment-1677484817)
Also, a developer may be moving the git folder at any time.

💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1293586086)
Done. I like that the assertion forces tests to make an initial setmocktime call before calling bumpmocktime.

✅ fanquake closed an issue: "wallet_importdescriptors.py: can't decode bytes in position 228861-228863: unexpected end of data"
(https://github.com/bitcoin/bitcoin/issues/28030)

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293608139)
> it's important for the documentation to at say what types it is allowed to be called with

I guess long term it could make sense to make `RPCArg::Type` an enum of C++ types and then have the documentation say that the type passed to `Arg<Type>(i)` must match exactly the corresponding `RPCArg::Type`.

For now, my recommendation would be to call `Arg<std::string>(i)` for `STR*` args, `Arg<bool>(i)` for `BOOL` args, and `Arg<double/(u)int${n}_t>(i)` for `NUM` args.

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1293610456)
Would it be fine to add "The `Type` passed to this helper must match the corresponding `RPCArg::Type`" to the doxygen of `Arg`?

🚀 fanquake merged a pull request: "test: locked_wallet, skip default fee estimation"
(https://github.com/bitcoin/bitcoin/pull/28232)

💬 MarcoFalke commented on pull request "ci: Run "macOS 11.0 [gui, no tests] [jammy]" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28265#discussion_r1293618511)
> Artifacts are subjects of 90 days [retention period](https://docs.github.com/en/actions/using-workflows/storing-workflow-data-as-artifacts). Therefore, I still lean to ghcr.io.

A re-build every 3 months seems perfectly fine. ghcr is *currently* free and unlimited, but I can't imagine this being the case forever. (The whole reason this pull was opened is because we relied on a service assuming it will be free and unlimited forever). Also, ghcr requires write access.

In any case, CI images
...

💬 iBeizsley commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1677542038)
> Secondly, on the basis of mempool consistency with miners, since full-rbf is the compatible policy in almost all cases, we should be supporting it by default if you take the position that we want to optimize for consistency with miners. (CC: @achow101)

If 37% of hash rate is mining full RBF, enabling full RBF is (currently) a move away from consistency with majority hash rate.

> Thirdly, all previous arguments in favor of full-rbf, such as multiparty transactions, wallets, user-expectations,
...

💬 hebasto commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1677548617)
Concept ACK.

👍 dergoegge approved a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1577100942)
reACK 9eac5a0529f869075f0331e40d322c34fc8fc2af

💬 MarcoFalke commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#discussion_r1293632896)
> Yeah, would you prefer it like this?

No preference. Currently it is wrong, so you'll have to remove it, or fix it, or close this pull.

👍 MarcoFalke approved a pull request: "test: refactor: support sending funds with outpoint result"
(https://github.com/bitcoin/bitcoin/pull/28264#pullrequestreview-1577126645)
lgtm, two nits only

💬 MarcoFalke commented on pull request "test: refactor: support sending funds with outpoint result":
(https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1293647505)
```suggestion
utxos = self.create_outpoints(self.nodes[0], outputs=[{node1_addr: 13},{node2_addr: 13},
])
```

💬 MarcoFalke commented on pull request "test: refactor: support sending funds with outpoint result":
(https://github.com/bitcoin/bitcoin/pull/28264#discussion_r1293649848)
```suggestion
def create_outpoints(self, node, *, outputs):
```

nit: I guess it is also fine to omit the named arg for node.

💬 MarcoFalke commented on issue "Regtest mode loses unspents after day":
(https://github.com/bitcoin/bitcoin/issues/28262#issuecomment-1677589589)
What are the exact steps to reproduce with mocktime?

📝 hebasto opened a pull request: "ci: Drop no longer needed `macos_sdk_cache`"
(https://github.com/bitcoin/bitcoin/pull/28269)
It has been cached in the Docker image since https://github.com/bitcoin/bitcoin/pull/27028.

💬 MarcoFalke commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#issuecomment-1677604972)
The CI uses the combine logs helper. Can you link to a CI output before and after?

🤔 MarcoFalke reviewed a pull request: "ci: Drop no longer needed `macos_sdk_cache`"
(https://github.com/bitcoin/bitcoin/pull/28269#pullrequestreview-1577164611)
lgtm. Can use `GLOBAL_TASK_TEMPLATE` now?

💬 hebasto commented on pull request "ci: Drop no longer needed `macos_sdk_cache`":
(https://github.com/bitcoin/bitcoin/pull/28269#issuecomment-1677619333)
> Can use `GLOBAL_TASK_TEMPLATE` now?

Thanks! Updated.