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

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

> nit: would `m_error` or (`m_error_info`) be a more appropriate name? E.g. in `bool()`, the meaning of `!m_error` is much more intuitive at first sight compared to `!m_info`, I think? (i.e. it's not really clear what it means to "not have info", whereas "not have error" is clear).

Calling it `m_error` would be misleading in later commits. The purpose of this field isn't to indicate the presence of an error. It happen
...

💬 achow101 commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#discussion_r1281126061)
`OP_VER` can be included here too:

```suggestion
opcode == OP_VERNOTIF ||
(fExec && opcode == OP_VER))
```

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1281161914)
> WriteImpl, ssKey and ssValue are all CDBBatch class members, I'm not sure passing these as parameters makes sense? And if so, I think ssKey should be const.

This gave me pause too, but it felt clearer to me this way. Let me know if you think passing them arguments here is more confusing.

> At first sight, I'm not entirely sure why ssKey and ssValue are CDBBatch members when they get resized and cleared in every operation, so perhaps cleaning that up is the better way to go, but I don't l
...

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1661120330)
Thank you for the review @stickies-v,

Updated e8a3a0a0d761c8544e20ffc3ee3d4c1029c44518 -> 8c4481ed3713931247e4cedcb5733d3598050eb7 ([cleaveLeveldbHeaders_1](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_1) -> [cleaveLeveldbHeaders_2](https://github.com/TheCharlatan/bitcoin/tree/cleaveLeveldbHeaders_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/cleaveLeveldbHeaders_1..cleaveLeveldbHeaders_2))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/b
...

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1661125511)
Rebased 5720741c14e617ed338f64361b23f4e66ec99e07 -> 7721ab9139013d70ef0f058754fabc5aaeda2246 ([kernelReturnOnAbort_2](https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort_2) -> [kernelReturnOnAbort_3](https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelReturnOnAbort_2..kernelReturnOnAbort_3))

* Fixed conflict with https://github.com/bitcoin/bitcoin/pull/27746

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1281197388)
Updated, and added explanation in the commit message.

```diff
- case BCLog::LogFlags::NONE: // this case is normally never hit
- return "";
+ case BCLog::LogFlags::NONE:
+ Assume(false);
```