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

Oh, I think I just forgot about prune locks added by indexes when I wrote this comment. If indexes are disabled, then the normal `GetPruneRange` function that controls pruning will allow pruning blocks between the snapshot and the tip.

But if indexes are enabled, because indexes prevent any blocks that haven't been indexed yet from being pruned, and because indexes currently just ignore the snapshot and don't take adv
...

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