💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1689871705)
In commit "RPC: Add height parameter to dumptxoutset" (5df60fb49cfae1e3a56b68e2f43ff0722f39abb7)

It's not safe to dereference `max_height` if snapshot_heights is empty. Would be good to either assert snapshot_heights is nonempty, or just leave target_index null in this case and not roll back if it is null.

💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1689671443)
In commit "RPC: Add height parameter to dumptxoutset" (5df60fb49cfae1e3a56b68e2f43ff0722f39abb7)

Seems like this accepts a height or a hash, so could mention this accepts block hashes and maybe call this height_or_hash or whatever the convention is

💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1689840218)
re: https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649335464

I don't think this is critical to fix but if you wanted to remove the race condition, the following change should work:

<details><summary>diff</summary>
<p>

```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -74,6 +74,22 @@ static GlobalMutex cs_blockchange;
static std::condition_variable cond_blockchange;
static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange);

+std::tuple<std::un
...

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689903917)
we do! fixed.

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689904434)
yes, updated.
thanks.

👍 instagibbs approved a pull request: "m_tx_download_mutex followups"
(https://github.com/bitcoin/bitcoin/pull/30507#pullrequestreview-2196845744)
ACK https://github.com/bitcoin/bitcoin/pull/30507/commits/1b732dbc3bd9c59303a774cf2095c2d6639dbe1d

💬 instagibbs commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689880324)
aside: we could throw an `Assume()` on the pointer before deref, since it's not set in a non-trivial way above

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689906830)
The rationale was to fill up `node[2]` block template before mining, but you are right, this is not needed.
removed.

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689908839)
No any rational for this, it's just to reduce the wait time.
removed to use the default.

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689909451)
updated to your suggestion.
Thanks!

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689910236)
fixed.
thanks

💬 dergoegge commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689914788)
(If you're gonna do this) `Assert` would make more sense, since we'd never want to hit the deref if it fails

⚠️ fanquake opened an issue: "guix: update LIEF from 0.13.2 to 0.15.x"
(https://github.com/bitcoin/bitcoin/issues/30520)
Updating our [`python-lief`](https://github.com/bitcoin/bitcoin/blob/fa0b5d68823b69f4861b002bbfac2fd36ed46356/contrib/guix/manifest.scm#L150) package in Guix (as well as the package [installed in the CI](https://github.com/bitcoin/bitcoin/blob/fa0b5d68823b69f4861b002bbfac2fd36ed46356/ci/lint/04_install.sh#L53)), would be useful for:
* [Enabling the `RELOC_SECTION` test](https://github.com/bitcoin/bitcoin/blob/fa0b5d68823b69f4861b002bbfac2fd36ed46356/contrib/devtools/test-security-check.py#L89)
...

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