💬 ChristopherA commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1646904832)
Concept ACK. Current limits on op_return force people to use hacks to store data hiding in op_codes. Using op_return is more `honest’. In my own case, current op_return limits keep me from posting a signed hash plus some metadata (<256 bytes) and thus I must use Taproot tricks instead.

🤔 jonatack reviewed a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1542308295)
Approach ACK b89567f51ade926af8c918e4787046b7ccec8eb0

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271496876)
29e31d8

<details><summary><code>ParseSighash()</code> suggestions</summary><p>

```diff
--- a/src/core_io.h
+++ b/src/core_io.h
@@ -47,7 +47,7 @@ bool DecodeHexBlockHeader(CBlockHeader&, const std::string&
-util::Result<int> ParseSighash(const std::optional<std::string>& sighash);
+[[nodiscard]] util::Result<int> ParseSighash(const std::optional<std::string>& sighash);

--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -245,7 +245,7 @@ bool ParseHashStr(const std::string& strHex,
...

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271533741)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 this ought to be added to `core_read.cpp` too; see full iwyu suggestions at https://cirrus-ci.com/task/4627440203464704?logs=ci#L1786

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271543977)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 When storing a `Result` type, I think it's clearer to name it `result` (seems to be becoming a convention). In particular, `util::ErrorString(parsed_sighash)` is a little confusing.

```cpp
const auto result{ParseSighash(sighash.isNull() ? std::nullopt : std::make_optional(sighash.get_str()))};
if (!result) {
throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(result).original);
}
...

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271540053)
https://github.com/bitcoin/bitcoin/commit/29e31d894bd8bedb55c3a5a2d4d2681ba10a9b88 (feel free to ignore) I wouldn't find review harder if the missing brackets were added

```cpp
if (v.isStr()) {
strHex = v.getValStr();
}
if (!IsHex(strHex)) {
throw std::runtime_error(strName + " must be hexadecimal string (not '" + strHex + "')");
}
```

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271547869)
> I should probably add a tutorial-style document that describes how to use the Result class with standalone functions that return util::Result, chained functions that return util::Result values of the same type, and chained functions that return util::Result values of different types.

I think this would be valuable, either in `util/result.h` directly or in `doc/developer-notes.md`.

⚠️ jonatack opened an issue: "p2p_getaddr_caching.py failure in TSan CI"
(https://github.com/bitcoin/bitcoin/issues/28133)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

https://cirrus-ci.com/task/5471865133596672?logs=ci#L3981


### Expected behaviour

https://cirrus-ci.com/task/5471865133596672?logs=ci#L3981


### Steps to reproduce

https://cirrus-ci.com/task/5471865133596672?logs=ci#L3981


### Relevant log output

https://cirrus-ci.com/task/5471865133596672?logs=ci#L3981

```
test 2023-07-22T19:16:17.696000Z TestFramework (ERROR): Assertion fa
...

💬 emc99 commented on pull request "Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1646954964)
When you say "by default", do you mean that full-rbf would come by default as part of IBD or when you update Bitcoin Core? When would full-rbf be "by default"?

💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1646955146)
Among free options, CircleCI looks better than others, IMO:
- https://circleci.com/docs/plan-free/
- https://circleci.com/pricing/

> Personally, I don't use macOS nor Windows, and I can't recommend it to anyone, so I don't mind if the tasks are removed.

We are responsible for quality of every release we are [shipping](https://bitcoincore.org/en/download/). And the user is free to make their own choice.

💬 emc99 commented on pull request "fuzz: Re-enable symbolize=1 in ASAN_OPTIONS":
(https://github.com/bitcoin/bitcoin/pull/28124#issuecomment-1646955442)
How do you fuzz? What does it mean to 'fuzz'?

💬 emc99 commented on pull request "build: pass sanitize flags to instrument libsecp256k1 code":
(https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1646956148)
> Maybe mark as draft for as long as CI is red?

When does CI happen? Why would CI be red in this case?

📝 jonatack opened a pull request: "rpc, util: deduplicate AmountFromValue() using util::Result"
(https://github.com/bitcoin/bitcoin/pull/28134)
See commit messages for details.

💬 TheCharlatan commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1646962287)
Updated b89567f51ade926af8c918e4787046b7ccec8eb0 -> a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240 ([kernelRmUnivalue_4](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_4) -> [kernelRmUnivalue_5](https://github.com/TheCharlatan/bitcoin/tree/kernelRmUnivalue_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmUnivalue_4..kernelRmUnivalue_5))

* Addressed @jonatack's [comment](https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1271533741), fixing IWYU.
* Addres
...

💬 jonatack commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1646964712)
ACK a3774d1b2a5ce9aa6d6d3cedc2c9b9a5d2f68240

💬 petertodd commented on pull request "Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1646966801)
> When you say "by default", do you mean that full-rbf would come by default as part of IBD or when you update Bitcoin Core? When would full-rbf be "by default"?

This pull-req has nothing to do with Initial Block Download (IBD).

It simply changes the default for the `-mempoolfullrbf` option to true/enabled. Previously the default was false/disabled. Users who update Bitcoin Core to a version containing this change would by default propagate and mine full-rbf replacements unless they had ch
...

💬 theStack commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1271567848)
`salt` is currently not used anywhere.

👋 jonatack's pull request is ready for review: "rpc, util: deduplicate AmountFromValue() using util::Result"
(https://github.com/bitcoin/bitcoin/pull/28134)

📝 bitcoinfinancier opened a pull request: "v 25.0.3"
(https://github.com/bitcoin/bitcoin/pull/28135)
semgrep ci integration for security and optimized images with AI to reduce storage and enhance quality.

💬 jonatack commented on pull request "Remove C-style const-violating cast, Use reinterpret_cast":
(https://github.com/bitcoin/bitcoin/pull/28127#issuecomment-1646980929)
Concept ACK