💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2248232775)
Rebased. This PR branch is the current `cmake-staging` [branch](https://github.com/hebasto/bitcoin/commit/dc490dae00d66b8e7b07158d1fc7adf53b4cce43) with 2 top commits dropped.

Also some comments have been addressed.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1689974792)
@theuni @maflcko

Can you confirm please that the recent [push](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2248232775) resolved this issue?

💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2248249525)
Rebased to deal with merge conflicts

👍 stickies-v approved a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2196273603)
ACK fa93c830d8674d01731868c1224190c3cd02d411

Left a couple of suggestions but nothing blocking.

Returning an early "Parse error" for malformed txids instead of silently ignoring them is a better user interface, and the underlying hex parse refactoring is a very welcome improvement.

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689546827)
in eeeeb0474d439c8d7e5ea9a680119245a455b5a0: should now be "please use the safer FromHex" since recent force push

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689539020)
Would be more robust to use `.value()` here. Suggestion that also skips unnecessary assignment to `TmpL`:

<details>
<summary>git diff on fa479be4d8</summary>

```diff
diff --git a/src/test/uint256_tests.cpp b/src/test/uint256_tests.cpp
index 52a8dbc200..430fc14892 100644
--- a/src/test/uint256_tests.cpp
+++ b/src/test/uint256_tests.cpp
@@ -168,12 +168,10 @@ BOOST_AUTO_TEST_CASE(methods) // GetHex SetHexDeprecated FromHex begin() end() s
// Verify previous values don't persist wh
...

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689990297)
Since this is a potentially breaking interface change for downstream applications, adding release notes might be appropriate?

<details>
<summary>git diff on fa2d94a01c</summary>

```diff
diff --git a/doc/release-notes-30482.md b/doc/release-notes-30482.md
new file mode 100644
index 0000000000..6eb8af3f69
--- /dev/null
+++ b/doc/release-notes-30482.md
@@ -0,0 +1,6 @@
+Updated REST APIs
+-----------------
+- Parameter validation for `/rest/getutxos` has been improved by
+ rejectin
...

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689596319)
nit: should we add this docstring to `uint256S` too?

💬 sr-gi commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#issuecomment-2248269822)
Rebased to deal with CI failing.

c76f4c6dab267b8f83c75e44939f77d237251c1a has been amended to include `__file__` in the `ChainTiebreaksTest` constructor, as required by any subclass of `BitcoinTestFramework` since https://github.com/bitcoin/bitcoin/pull/30463

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2248276315)
Rebased to deal with CI failing.

c76f4c6dab267b8f83c75e44939f77d237251c1a has been amended to include `__file__` in the `P2PSeedNodes` constructor, as required by any subclass of `BitcoinTestFramework` since https://github.com/bitcoin/bitcoin/pull/30463

💬 fanquake commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1690009227)
Thanks, had a look at this, and ran into issues with at least leveldb, which we'd first have to fix in the subtree, i.e:
```bash
In file included from leveldb/util/comparator.cc:14:
./leveldb/util/no_destructor.h:40:17: error: 'aligned_storage<8, 8>' is deprecated [-Werror,-Wdeprecated-declarations]
40 | typename std::aligned_storage<sizeof(InstanceType),
| ^
leveldb/util/comparator.cc:71:47: note: in instantiation of template class 'leveldb::NoDestructor<leveldb
...

💬 fanquake commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1690012798)
Thanks, added.

💬 fanquake commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1690022414)
There is an up-upstream issue / PR for this. https://github.com/google/leveldb/issues/1013 & https://github.com/google/leveldb/pull/896.

💬 ryanofsky commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2248316407)
Mentioning the history of this change in the PR description is helpful, but it would be clearer if it said what the change is and what effects it has.

From code comments in this PR and https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2188840483, it seems like the main intended effect of this change is to prevent github tasks from running twice in PRs and causing the following output:

> <img width="1038" alt="Scherm­afbeelding 2024-06-25 om 14 48 34" src="https://github.com/bitcoi
...

💬 fanquake commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2248331845)
Guix Build (aarch64):
```bash
b2617f05ed438479493daac5d6faa637d9bc8368186f41db270392ae8bcc2ff1 guix-build-c399c80a09a3/output/aarch64-linux-gnu/SHA256SUMS.part
255827ffdae24ce4099885caa4517a0d05e883adc49f92bf36226afa910d1b6b guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu-debug.tar.gz
299e1401c19e901c30096b870b34c342318d08c89bc3b6ac02a82e718ddc41ad guix-build-c399c80a09a3/output/aarch64-linux-gnu/bitcoin-c399c80a09a3-aarch64-linux-gnu.tar.gz
dea5a7
...

💬 maflcko commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#issuecomment-2248337297)
re-ACK e3edaccd9deb2da50be70d2d8768eca8821785c7

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690047209)
Sure, done for both pulls.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690048284)
Yes, done.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690049630)
Good catch. Fixed the typo!

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690051167)
Ok, done in both places