💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1211560160)
Good catch on the obsolete link. The proposed new link doesn't seem relevant, however. I didn't immediately see a good replacement. For less future maintenance, maybe just drop this sentence from the doc.

💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1570046217)
> But also we should probably figure out a better way to deal with the conversions in general.

I think it was an unfortunate decision to hardcode parameter information into the client, and could think of various alternatives.

One alternative could be to avoid the need to process arguments on the client side at all and and add a new `exec` RPC method that takes an `args` array of strings and an optional `stdin` string parameter to figure out on the server side where there is full type infor
...

💬 MarcoFalke commented on issue "Indicate RBF replaceability, also after transactions have been confirmed ":
(https://github.com/bitcoin/bitcoin/issues/27794#issuecomment-1570106061)
I didn't get the use case. You can simply skip your action that checks the rbf status when the transaction is confirmed.

💬 torkelrogstad commented on issue "Indicate RBF replaceability, also after transactions have been confirmed ":
(https://github.com/bitcoin/bitcoin/issues/27794#issuecomment-1570137331)
I have a set of actions S, that every transaction needs to go through.

If a transaction T does _not_ signal BIP125-replaceability, I want to run S when T is in the mempool. If T _does_ signal BIP125-replaceability, I want to run S when T is confirmed.

Currently it is not possible to find out (through the RPC interface, at least) whether or not T signals BIP125-replaceability once it is confirmed.

💬 sipa commented on issue "Indicate RBF replaceability, also after transactions have been confirmed ":
(https://github.com/bitcoin/bitcoin/issues/27794#issuecomment-1570148419)
The `bip125-replaceable` field does not indicate whether the transaction itself signals replaceability, but whether replaceability rules currently apply to it. This is a function of both the transaction itself, and any unconfirmed ancestors it may have. As a result, it's expected to change over time.

And for confirmed transactions, the answer to the question "do replaceability rules currently apply to it?" is trivially no.

💬 jonatack commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1211651578)
It wasn't necessary to do here (no global scope name collisions), but I think it would make sense. Per `doc/developer-notes.md`:

```
- Prefer `enum class` (scoped enumerations) over `enum` (traditional enumerations) where possible.

- *Rationale*: Scoped enumerations avoid two potential pitfalls/problems with
traditional C++ enumerations: implicit conversions to `int`, and name clashes
due to enumerators being exported to the surrounding scope.
```

💬 fanquake commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1570173602)
Related to #7996.

💬 jonatack commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1570183892)
Rebased 🥉

@ccdle12 Thanks! Mind re-acking? As to motivation, I think it's a mix of both reasons you mentioned, with the first one being the direction to go when doing so -- see `doc/design/libraries.md` and merged changes like c741d748d4.

💬 willcl-ark commented on pull request "ci: Enable float-divide-by-zero check":
(https://github.com/bitcoin/bitcoin/pull/27778#issuecomment-1570188617)
utACK fa3ab4520317f48d4700b81dab023c4e639bbd68

The changes are straight forward-enough and I agree with the rationale, but I've been unable to test them locally due to unrealted build errors:

<details>
<summary>Details</summary>

```log
Making install in src
make[1]: Entering directory '/home/will/src/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
make[2]: Entering directory '/home/will/src/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src'
rm -f libtest_util.a
/u
...

💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1570229861)
Rebased for https://github.com/bitcoin/bitcoin/pull/27571 🥈

💬 hebasto commented on pull request "ci: Enable float-divide-by-zero check":
(https://github.com/bitcoin/bitcoin/pull/27778#issuecomment-1570241144)
> I've been unable to test them locally due to unrealted build errors:

Mind providing more detailed steps to reproduce the failure?

💬 MarcoFalke commented on pull request "ci: Enable float-divide-by-zero check":
(https://github.com/bitcoin/bitcoin/pull/27778#issuecomment-1570253029)
It probably depends on the CPU model to reproduce the unrelated failure?

💬 fanquake commented on pull request "ci: Enable float-divide-by-zero check":
(https://github.com/bitcoin/bitcoin/pull/27778#issuecomment-1570253322)
> Mind providing more detailed steps to reproduce the failure?

Clearly those failures are unrelated to this change.

💬 fanquake commented on pull request "ci: Enable float-divide-by-zero check":
(https://github.com/bitcoin/bitcoin/pull/27778#issuecomment-1570261798)
Checked that this found the previously fixed related failure, on aarch64, running the ASAN job:
```bash
test/argsman_tests.cpp(169): Entering test case "util_CheckValue"
validation.cpp:2839:5: runtime error: division by zero
#0 0xaaaab0a4c198 in Chainstate::ConnectTip(BlockValidationState&, CBlockIndex*, std::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2839:5
#1 0xaaaab0a506a0 in Chainstate::ActivateBestChainStep(BlockValidation
...

🚀 fanquake merged a pull request: "ci: Enable float-divide-by-zero check"
(https://github.com/bitcoin/bitcoin/pull/27778)

💬 Sjors commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1211763951)
> @Sjors this is bash :)

*bashes head against table*

💬 fanquake commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1570295375)
Note that you'l have to drop the `lazy_bind` check (we'll follow up with a `fixup_chains` test later). I've got a branch here with that change, which Guix builds successfully: https://github.com/fanquake/bitcoin/tree/27676_minus_lazy_bind.

Guix Build:
```bash
2da11646ff030cbd9207027e5af33ec77e8dc30fa4811fb47e6c2201da9b54e5 guix-build-36d66d7df78e/output/arm64-apple-darwin/SHA256SUMS.part
e1c5a81e97a942ace7c57fd7c12b8f45287939dd193418c435ac806d28d76b28 guix-build-36d66d7df78e/output/arm64
...

💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1211789138)
Good question. The Socks5 server we use in the test framework by default disconnects immediately after the handshake and sometimes would not register as a connected peer:

https://cirrus-ci.com/task/6075793026056192?logs=ci#L3820-L3830

💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1570323701)
Thanks for the review Will, nits addressed and pushed

💬 torkelrogstad commented on issue "Indicate RBF replaceability, also after transactions have been confirmed ":
(https://github.com/bitcoin/bitcoin/issues/27794#issuecomment-1570339251)
Ok, thanks for the input. IMO what I've laid out here is something consumers of this API would care about. The Core API in general is very hard to work with, this being one example.