🤔 jonatack reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1541828147)
First-pass ACK 775b54e88107b0b976bf995e607926013fa9bc42

In https://github.com/bitcoin/bitcoin/commit/332e847c9ec0241efd9681eee3b03ff819aaddc3 and 4d995d3fa66fbc3eb87c6627e5ba1b2a809402a4, I wonder if some of the custom operator (i.e. move) definitions should have a noexcept-specification. Also, notating any methods where it would be incorrect if the return value isn't checked (e.g. for error-handling) and optionally getter-like pure functions with `nodiscard` may aid reviewers / readers of th
...

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271094067)
332e847c9ec tuple not needed per iwyu in tidy ci https://cirrus-ci.com/task/6540065057275904?logs=ci#L20325, while touching could add the others

```diff
+#include <assert.h>
#include <memory>
#include <optional>
-#include <tuple>
#include <utility>
#include <vector>
+#include <new>
+#include <type_traits>
```

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1271096350)
https://github.com/bitcoin/bitcoin/commit/332e847c9ec0241efd9681eee3b03ff819aaddc3 per iwyu in https://cirrus-ci.com/task/6540065057275904?logs=ci#L20305

```diff
#include <util/result.h>
-#include <util/string.h>
+#include "util/translation.h"
+
+#include <algorithm>
+#include <initializer_list>
+#include <iterator>
```

💬 besoeasy commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646265590)
@manfreddd can you share your CPU Usage screenshot ?

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1646270585)
Added a missing check in the fuzz target since `GetExecStackSize()` now returns an optional.

💬 manfreddd commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646284658)
Sure, enclosing the Task Manager window screenshot (in Portuguese).

💬 besoeasy commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646325159)
@manfreddd you using SSD ? your disc seems slow

💬 besoeasy commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646325611)

@manfreddd you using SSD ? your disc seems slow

+ your antivirus might be closing bitcoin process for p2p communication

💬 manfreddd commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646337045)
Enclosing debug file from last Bitcoin Core run.
[debug.log](https://github.com/bitcoin/bitcoin/files/12135141/debug.log)

🤔 vostrnad reviewed a pull request: "script: add description for the functionality of each opcode"
(https://github.com/bitcoin/bitcoin/pull/27109#pullrequestreview-1541936673)
The more I look at this (and especially after the switch to notation), the more I think the descriptions should each be on a separate line above the opcode in a block comment (`/** ... */`). It would allow for longer descriptions where needed without ever exceeding the soft 100-per-line character limit.

The block comments could now include (where appropriate):
* Both a textual description and notation (frankly, I find the notation-only comments quite difficult to read).
* Historical context
...

📝 luke-jr opened a pull request: "Bugfix: RPC: Remove quotes from non-string oneline descriptions"
(https://github.com/bitcoin/bitcoin/pull/28123)
Various JSON Object parameters had a `oneline_description` with quote characters. Fix those, and extend `rpcdoccheck` to detect them.

Also, slightly improve GBT's oneline description for template_request.

💬 theStack commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1271193421)
off-by-one, the "message index wrong" kind of error type (error=12) is currently not tested:
```suggestion
for (unsigned error = 0; error <= 12; ++error) {
```
(to avoid issues like this, could add an enum with error types and give the highest error sth like ERROR_HIGHEST, to be used in the loop counter, but that's probably overkill)

👋 luke-jr's pull request is ready for review: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963)

💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-1646444551)
Rebased

👍 MarcoFalke approved a pull request: "Bugfix: RPC: Remove quotes from non-string oneline descriptions"
(https://github.com/bitcoin/bitcoin/pull/28123#pullrequestreview-1542053175)
review ACK 5e3e83b005518659a69916c373b808da27e51791

💬 MarcoFalke commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271252144)
nit: Could use `STR_INTERNAL_BUG`?

```suggestion
STR_INTERNAL_BUG(strprintf("Internal bug detected: non-string RPC arg \"%s\" quotes oneline_description:\n%s\n",
m_names, m_opts.oneline_description,

)};
```

💬 MarcoFalke commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1271252337)
I think you can remove the IsNull check below? Also, does this need release notes?

💬 MarcoFalke commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1646498972)
Can you share the output of `gettxoutsetinfo muhash`, which would allow to compare the result to one on another machine to detect potential leveldb corruption.

💬 hebasto commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1646499733)
Suggesting an alternative implementation that is based on the following idea:
```diff
--- a/src/core_read.cpp
+++ b/src/core_read.cpp
@@ -242,10 +242,10 @@ bool ParseHashStr(const std::string& strHex, uint256& result)
return true;
}

-int ParseSighashString(const UniValue& sighash)
+int ParseSighashString(const std::optional<std::string>& sighash)
{
int hash_type = SIGHASH_DEFAULT;
- if (!sighash.isNull()) {
+ if (sighash.has_value()) {
static std::map<
...

📝 MarcoFalke opened a pull request: "fuzz: Re-enable symbolize=1 in ASAN_OPTIONS"
(https://github.com/bitcoin/bitcoin/pull/28124)
Looks like this fixed itself somehow and is no longer reproducible?