💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2247865660)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/278.

🤔 stratospher reviewed a pull request: "net: Log accepted connection after m_nodes.push_back; Fix intermittent test issue"
(https://github.com/bitcoin/bitcoin/pull/30512#pullrequestreview-2196772985)
tested ACK fa3ea3b.

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#discussion_r1689861943)
Added.

💬 fanquake commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2248024050)
> i'd be okay with skipping the check for bitcoin-util: it's the least relevant binary for fortification (no network access, not even file format access). Could reconsider it later if it actually gains some useful functionality 😄

I think I agree, and I've added that skipping now.

👋 fanquake's pull request is ready for review: "security-check: test for `_FORTIFY_SOURCE` usage in release binaries"
(https://github.com/bitcoin/bitcoin/pull/27038)

💬 sipa commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2248056425)
utACK c399c80a09a393d38368a44ef04753e9f62350f0

👍 ryanofsky approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2196517706)
Code review ACK 27767184aaa7fa94600c70334cc4122e25872ff6.

Main change since last review is documenting dumptxoutset better and making default behavior be to dump the latest snapshot that can be loaded, instead of the current utxo set. Also improved other documentation and added NetworkDisable class to more reliably restore network state in case of error.

Left some additional suggestions but they aren't very important

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