💬 Ayms commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1660871572)
Not on mobile any longer, then I read the 10 years article, counterparty was right, we are 10 years late, then many switched on eth
The 40 or 80B limit was based on storing a hash or two, surprisingly nobody thought at that time that you must store a signed hash as @ChristopherA and myself wants to do and probably many others, plus some metadata, which is not possible today unless you know a miner, storing a hash or two is of no use, or just leads to a very centralized system, like eth sidechai
...

👍 theStack approved a pull request: "p2p, rpc, test: `addnode` improv + add test coverage for invalid command"
(https://github.com/bitcoin/bitcoin/pull/26366#pullrequestreview-1557670704)
re-ACK c2349d6a04832d9c96a8746fc588bae8fc85376d

The only change since my last ACK was adapting the first commit's subject (s/getpeerinfo/addnode/). Sorry for late re-review, I must have missed the force-push two months ago.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281063486)
Done

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281063610)
Added a comment

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1281063722)
Added a comparison

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1660955989)
> I still suggest changing fc810c1 so that dumpwallet does not use the read-only format by default, and instead change the tests use it.

Since dumping should be a read-only operation, I think it's better to use the read-only database since it definitely won't write to it like BDB does. Also it provides a good mechanism for testing.

💬 fjahr commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1660993936)
Post-merge code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9

I also synced and kept a node running with the status of my previous ACK and didn't see anything unusual.

💬 jonatack commented on pull request "p2p, rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281087542)
While touching this line, mentioning getpeerinfo doesn't seem particularly relevant as addnode is generally used to add a trusted peer that you already know (e.g. in order to avoid any eclipse situation).

```suggestion
{"node", RPCArg::Type::STR, RPCArg::Optional::NO, "The node's address"},
```

💬 jonatack commented on pull request "p2p, rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281091853)
```suggestion
else if (command == "remove")
```

and could also fix the conditional bracket formatting (run clang-format)

💬 jonatack commented on pull request "p2p, rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1281083516)
nit, this seems a bit clearer and specific

```suggestion
assert_raises_rpc_error(-1, 'addnode "node" "command"', self.nodes[0].addnode, node=ip_port, command='abc')
```

🤔 jonatack reviewed a pull request: "p2p, rpc, test: `addnode` improv + add test coverage for invalid command"
(https://github.com/bitcoin/bitcoin/pull/26366#pullrequestreview-1557715169)
ACK c2349d6a04832d9c96a8746fc588bae8fc85376d

some non-blocking suggestions

💬 fjahr commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1660999249)
Concept ACK

💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1661000638)
> I think the CI clang-tidy task is completely separate from the CI lint task.

I guess in my mind they are the same thing. `clang-tidy` is just a more sane form of linting infrastructure, compared to a lot of what we currently use.

> It requires a full build system,

Can you elaborate on why this is a problem? Any rust code also requires a full build system (cargo) and npm-like dependency management (crates). In the clang-tidy case, the build system is packages we'd already be installing
...

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1557519459)
Updated 775b54e88107b0b976bf995e607926013fa9bc42 -> 1de05ef9190202f04f8cbe7746a47cbd66ab540c ([`pr/bresult2.35`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.35) -> [`pr/bresult2.36`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.36), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.35..pr/bresult2.36)) making suggested changes

I still want to do more work to make the result class to enforce more safety with bool/optional/pointer types as discussed htt
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1280960077)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276382028

> > I am also thinking of adding a util::Messages{Result&&} helper.
>
> That sounds like a worthwhile improvement of the ergonomics here.

Thanks, added this

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1280960230)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271094067

> [332e847](https://github.com/bitcoin/bitcoin/commit/332e847c9ec0241efd9681eee3b03ff819aaddc3) tuple not needed per iwyu in tidy ci https://cirrus-ci.com/task/6540065057275904?logs=ci#L20325, while touching could add the others

Thanks, updated includes

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1280960587)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1277588274

> nit: would it be more idiomatic to use `has_value()` here?

I don't think it's more idiomatic but the suggestion seems fine so I adopted it.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1280961601)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271096350

> [332e847](https://github.com/bitcoin/bitcoin/commit/332e847c9ec0241efd9681eee3b03ff819aaddc3) per iwyu in https://cirrus-ci.com/task/6540065057275904?logs=ci#L20305

Thanks, updated includes

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1280960940)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1277584643

> Would it make sense to rename this to `emplace`, to keep the interface in line with `optional` and `expected`?

This isn't an actually an emplace method, it's an assignment method. An emplace method for a `Result<T>` object would take arguments that would be accepted by one of `T`'s constructors, and construct a new `T` object in place with them.

By contrast, this method doesn't accept arguments that could be forw
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1280961487)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276417417

> It strikes me as unfortunate that the move constructor cannot move the failure values across different result value types, meaning the failure needs to be passed in as a separate argument. At the same time the user would be allowed to return a `{std::move(result), util::Error{Untranslated("str error")}`, potentially without the user noticing that this will not move the failure value ~and instead initialize a `Monostate
...