💬 sdaftuar commented on pull request "test: Add test to check tx in the last block can be downloaded":
(https://github.com/bitcoin/bitcoin/pull/27695#issuecomment-1553197793)
ACK fa4c16b186a34f4172deda617166813c8cb92c59

💬 kallerosenbaum commented on issue "bitcoin-qt fails to parse binary psbt file":
(https://github.com/bitcoin/bitcoin/issues/27697#issuecomment-1553202155)
Thanks for you quick responses. Will use 25rc2 instead then.

🚀 achow101 merged a pull request: "wallet: fix deadlock in bdb read write operation"
(https://github.com/bitcoin/bitcoin/pull/27556)

💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1553225738)
> I think it would also be good to continue serving txs from vExtraTxnForCompact (and to use them for resolving orphans?) to help address the https://github.com/bitcoin/bitcoin/pull/17303#issuecomment-550460346, but I don't think that needs to be a prerequisite for dropping mapRelay.

I don't think the privacy issues raised by not relaying replaced transactions are significant at all. There are many ways to use transaction relay behavior to infer the network topology, and I think there are do
...

💬 fanquake commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1553226812)
@instagibbs this conflicts with #27626, thoughts?

💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1553231704)
Thanks everyone for reviewing! Here's what have changed:

- `get_utxo` will not return immature coinbase utxos anymore
- We won't use the `confirmation` count from utxo object since it may have outdated. We will use current block height + utxo's height for it
- changed `get_utxos` to use current block height to compute the confirmation count instead of using the value from utxo object, same logic from `get_utxo`.

💬 sdaftuar commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1197968591)
The `NodeClock::now()` time can go backwards, right? If so I think that could pose some issues if we're primarily relying on system time in order to determine whether transactions should be available for relay. Can we use a steady clock instead?

💬 sdaftuar commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1553249207)
Concept ACK on getting rid of the recently announced filter. I think we'd need to use a steady clock to avoid random failures for no good reason, is that easy to do?

💬 fanquake commented on issue "One core in CPU usage rate remains at 100% for a long time, causing serious delays in new blocks and forks":
(https://github.com/bitcoin/bitcoin/issues/27623#issuecomment-1553254260)
@huzhenyuan v24.1 is now available: https://github.com/bitcoin/bitcoin/releases/tag/v24.1.

👍 ryanofsky approved a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1432884414)
Code review ACK 861582b4087f1602e7115ee4a70ef7a7263eb36b. Just suggested changes since last review. (Thanks!)

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1197937840)
In commit "refactor: Split util::insert into its own file" (d492a2e5c88257cb14c417615dba525ce2cd9cfc)

Does this file need to `#include <set>`. Just wondering if IWYU would complain.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1197984670)
In commit "kernel: Add fatalError method to notifications" (861582b4087f1602e7115ee4a70ef7a7263eb36b)

I think it would make sense to get rid of this callback now that there is a notifications object. Following seems to work:

```diff
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index 41056272f549..3a055c8e0bc7 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -200,10 +200,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const C
...

💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1553274722)
At first glance, this doesn't seem to interfere in any meaningful way with #27626

I promise to review this as a follow-up cleanup :pray:

💬 fanquake commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1553277212)
> I promise to review this as a follow-up cleanup 🙏

Ok. This could be drafted for now / rebased on top of #27626?

👍 pinheadmz approved a pull request: "test: fix intermittent issue in `feature_bip68_sequence`"
(https://github.com/bitcoin/bitcoin/pull/27177#pullrequestreview-1432988025)
ACK ee018b25af9477050839826c27a885e7211eed14

Great work on this, nice and clean now ;-)

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK ee018b25af9477050839826c27a885e7211eed14
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmRmTNQACgkQ5+KYS2KJ
yTqJsBAAqV8ZWwsCyWxK8THFr3lc5gQWcgpERH4DHAXWFpMn1WGMBLn0aXs6VTMh
V5qXjM76piqQsQcJyLEli5r4zgW0hDF7YdNtLTwPNPEnMYqvI6uxj1mUMochZEXn
dgk9vhhQzRV4s/sixPNolV6Pt6
...

💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1198003218)
This line surprised me (test fails without it) then I saw [achows comment](https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1164465825) so I guess it does make sense to require the miniwallet's node to be running any time we need to send a tx with it. I wondered if maybe you could use `rescan_utxos` with any already-running node to update utxo confirmation count, but I guess this is probably the cleanest way

💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1198005686)
micro-nit: its called `block_height` in the other function. no need to fix unless you have to retouch

📝 fanquake opened a pull request: "random: drop syscall wrapper usage for getrandom()"
(https://github.com/bitcoin/bitcoin/pull/27699)
This requires a linux kernel of `3.17`+, which seems entirely
reasonable. `3.17` went EOL in 2015, and the last supported `3.x` kernel
(`3.16`) went EOL > 4 years ago, in 2020. For reference, the current
oldest maintained kernel is `4.14` (released 2017, going EOL Jan 2024).

Support for `getrandom()` (and `getentropy()`) was added to
glibc `2.25` https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00079.html:
> * The getentropy and getrandom functions, and the <sys/random.h> header

...

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198025552)
Seems wrong not to include it, will do so.

💬 brunoerg commented on pull request "fuzz: net, add `recv_flood_size`, `prefer_evict` in `ConsumeNode`":
(https://github.com/bitcoin/bitcoin/pull/27678#issuecomment-1553339959)
@dergoegge I just removed `perfer_evict` from `ConsumeNode`, kept `recv_flood_size` and added target for `MarkReceivedMsgsForProcessing`.

`MarkReceivedMsgsForProcessing` uses `m_recv_flood_size` and plays with `vRecvMsg` which may be filled by `ReceiveMsgBytes`.

If it makes sense for a PR scope, I can update the description and we can go ahead this way here. Otherwise, it's ok for me to close it for now and add `recv_flood_size` and `prefer_evict` alongside with more appropriate net target
...