💬 jonatack commented on pull request "rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet":
(https://github.com/bitcoin/bitcoin/pull/27757#discussion_r1211523574)
See the diff [here](https://github.com/jonatack/bitcoin/commit/56ec947e698a8d73d147c0a28130a67326678541#diff-7b13e03c457b62251e26d2c366180c262a3fe9da45995277a9a9c1c5abbaae6cR484) in https://github.com/jonatack/bitcoin/commits/2023-05-remove-deprecated-warning-fields making the same change a few weeks ago. I moved the move back to its original position before the deprecation.

⚠️ MatthewLM opened an issue: "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations"
(https://github.com/bitcoin/bitcoin/issues/27795)
### Please describe the feature you'd like to see added.

Distinguish between decimal and hex integers in the script ASM to avoid ambiguity. Hex integers/data can be prefixed with `0x` to avoid ambiguity.

### Is your feature related to a problem, if so please describe it.

When scripts are decoded into ASM, two different integers can be displayed identically, with one as hex and the other as decimal. `decodescript 0511121314150457c74942` produces ASM of `1112131415 1112131415` , despite the int
...

💬 MarcoFalke commented on issue "Indicate RBF replaceability, also after transactions have been confirmed ":
(https://github.com/bitcoin/bitcoin/issues/27794#issuecomment-1569997837)
A confirmed transaction can not be in the mempool, so can not be replaced in the mempool, whether with bip125 or not.

I don't see a use case to run the bip125 signal logic on confirmed transactions, and it shouldn't be hard to run it yourself if you just want to do it despite no use case?

💬 torkelrogstad commented on issue "Indicate RBF replaceability, also after transactions have been confirmed ":
(https://github.com/bitcoin/bitcoin/issues/27794#issuecomment-1570021896)
I have a use case for this:

I'm developing a service that receives bitcoin, with a Bitcoin Core wallet as the underlying wallet. When a transaction comes in I check whether or not it is BIP125 replaceable. If it is _not_, I run it through certain actions when first seeing it in the mempool. Upon receiving a confirmed transaction I need to run the transaction through those actions, if that has not already happened when seeing it in the mempool (i.e. for confirmed transactions that _was_ repla
...

🤔 jonatack reviewed a pull request: "doc: Tor: fix link & generalize onion getnodeaddresses RPC"
(https://github.com/bitcoin/bitcoin/pull/27719#pullrequestreview-1452944104)
Concept ACK

💬 jonatack commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#discussion_r1211576918)
This was done because there are many peers returned by `./src/bitcoin-cli getnodeaddresses 0 onion`, and at the time, only a few hundred I2P ones and a handful of CJDNS ones.

Perhaps it would be best to make the I2P doc like the Tor doc here, or combine both approaches for all three docs (Tor, I2P, CJDNS), i.e. something like:

```markdown
To fetch a number of onion peers that your node knows, for example 7 addresses, run
`bitcoin-cli getnodeaddresses 7 onion`. You may also run
`bitcoin-
...

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