💬 hernanmarino commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1122165135)
Removed extra parenthesis, and I agree with extra long lines, but as I said before, indentation was introduced after running the script suggested by @ jarolrod and are apparently ok according to clang format standard.

💬 furszy commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1122170162)
If the view is not accessible anymore, this changes aren't needed.

(same for the transaction table model ones)

💬 furszy commented on pull request "rpc: make importaddress compatible with descriptors wallet":
(https://github.com/bitcoin/bitcoin/pull/27034#discussion_r1122178962)
> > upgraded the legacy wallet into a descriptors wallet
>
> I think it's best to have only one way to go from a legacy wallet to a descriptor wallet (the migration RPC), so in the long run it's easier to keep testing this stuff.

yeah, agree.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1450711073)
@willcl-ark thanks, I added comments and release notes.

I also wrote a tiny testing package using [libjson-rpc-cpp](https://github.com/cinemast/libjson-rpc-cpp) to check against an "outside" JSON-RPC 2.0 implementation. The package is https://github.com/pinheadmz/jsonrpc-bitcoin and [seems to like the 2.0 implementation so far.](https://github.com/pinheadmz/jsonrpc-bitcoin#examples)

💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1122210010)
yeah, I'll do.

💬 mzumsande commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1122020763)
if this is only necessary for windows, would it be possible to hide it behind `#ifdef WIN32` so that the unit tests don't run 3*2=6 seconds longer for everyone else (which is quite a lot)?

💬 fanquake commented on issue "miniscript_stable fuzz timeout":
(https://github.com/bitcoin/bitcoin/issues/27147#issuecomment-1450764766)
Verified as fixed: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56332#c3

💬 fanquake commented on pull request "Make miniscript_{stable,smart} fuzzers avoid too large scripts":
(https://github.com/bitcoin/bitcoin/pull/27165#issuecomment-1450765037)
Verified as fixed: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56332#c3.

💬 sipa commented on issue "miniscript_stable fuzz timeout":
(https://github.com/bitcoin/bitcoin/issues/27147#issuecomment-1450771004)
@fanquake Wrong issue, actually; the original was about `miniscript_stable`: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56270

The one you link to was about `miniscript_smart`, but that is in fact also addressed by #27165.

💬 fanquake commented on issue "miniscript_stable fuzz timeout":
(https://github.com/bitcoin/bitcoin/issues/27147#issuecomment-1450773295)
> Wrong issue, actually;

Whoops. Yes, both ended up being addressed here.

💬 fanquake commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1450801900)
Rebased past #27172.

💬 fanquake commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1450803042)
Rebased past #27172.

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1122285875)
Thanks, I guarded the sleep for WIN32, cleaned up the lint and mentioned the undo data in OP.

💬 vostrnad commented on pull request "BIP324: Enable v2 P2P encrypted transport":
(https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1450843657)
Is there a reason the `v2transport` option should be off by default? Does it even need to exist at all? I was accidentally running this for a few days with v2 turned off and was confused why nodes advertising `P2P_V2` weren't actually using the v2 protocol. Everything works fine now.

Also, it would be nice to have the `transport_protocol_type` and `v2_session_id` fields displayed in the GUI, although that might possibly belong to a separate PR in the GUI repo (not sure what the development pr
...

💬 TheCharlatan commented on pull request "guix: pass `--enable-initfini-array` to release GCC":
(https://github.com/bitcoin/bitcoin/pull/27153#issuecomment-1450910713)
Guix builds [127c637](https://github.com/bitcoin/bitcoin/pull/27153/commits/127c637cf0a80e0ea68a7c5aaa088e5ccc9d3d13)

```
ae145877193079ceb71b86582b86294efba941d8898480e16920f8588bb25fca guix-build-127c637cf0a8/output/aarch64-linux-gnu/SHA256SUMS.part
b50822b488f79eba521dca2538b98ced15da3f175deeccce4030a6690bd4b077 guix-build-127c637cf0a8/output/aarch64-linux-gnu/bitcoin-127c637cf0a8-aarch64-linux-gnu-debug.tar.gz
a04d495f8338dc30afdc258df91708c20197f74fe7c84070d8e9ebc9d3924780 guix-bui
...

💬 ryanofsky commented on pull request "Fix BaseIndex::Commit false error":
(https://github.com/bitcoin/bitcoin/pull/26903#issuecomment-1450928417)
I agree 1c67b495cb2de76723c404f42d0bee219cc765c3 would fix the problem, but I think it suppresses errors too broadly. The `Commit()` function is called many places, so changing it to silently skip the `CustomCommit()` call and return `true` when it doesn't do anything could make it harder debug other problems in the future, or even know about them.

I think it would be better to apply a more targeted fix of just not calling `Commit()` when there is nothing to commit in the sync thread:

```d
...

💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1122337219)
```suggestion
// Write the first block; dbp=nullptr means this block doesn't already have a disk
// location, so allocate a free location and write it there.
FlatFilePos pos1{blockman.SaveBlockToDisk(block1, /*nHeight=*/1, chain, *params, /*dbp=*/nullptr)};
```
and similar below

💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1122332686)
```suggestion
constexpr int TEST_BLOCK_SIZE{81};
```

💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1122338428)
```suggestion
// Attempt, but fail, to save block 3 to original position of block 2
```

💬 LarryRuane commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1122400769)
Well, on non-windows, it _may_ have been modified, but within the same second. This is kind of a minor point (probably don't need to change anything), but if in the future there's a bug such that `blk00000.dat` was modified, this test might not catch it. I verified this by hacking the test such that the file _is_ modified, and about once in 10 or 20 runs, the timestamp didn't change. (This is on Ubuntu.)

But I guess that's okay, because if there is such a bug, it will likely be caught quickly
...