💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848453460)
Thinking back to [discussion](https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831104732) around how limiting prioritizetransaction for miners could backfire on the ecosystem in previous PR.

Encouraging miners to run with `-acceptnonstdtxn` means all *relayfee* limits for other transactions are effectively zero. But standardness touches upon other aspects of transactions as well (with more to be added in the future). So this is worse than only pushing miners to zero out *relayfee*-a
...

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848433819)
perf nit: Could remove this line if using `.insert` and checking the return value instead of `.contains` above as suggested here https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831063128.

The current version is slightly easier to read but does an *extra lookup*. I understand if you want to leave as-is for readability as the perf-hit is probably negligible.

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1849298256)
nit:
```suggestion
// Only MAX_DUST_OUTPUTS_PER_TX dust is permitted (on otherwise valid ephemeral dust)
```

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1849209897)
In 0f92fa2d468186a1526827a063f66e3042e028dc:
No longer used, should probably be removed there.

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848737688)
In 60e18bb2dcd2c16a64e01023b793b0494cd6ca64:

nit: (Resolved in later commit) - `.value_or(null_txid)` seems unnecessary? Suggestion:
```C++
BOOST_CHECK_EQUAL(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool), dust_non_spend_txid);
```
Changing to that convention makes the introduction of `null_txid` pointless.

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848750297)
Should further link `dust_index` with the value used by callers of the function, as current version has a hidden dependency.

<details>
<summary>
Sending as parameter
</summary>

```diff
diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp
index b362abfa1d..0a51ff411f 100644
--- a/src/test/txvalidation_tests.cpp
+++ b/src/test/txvalidation_tests.cpp
@@ -91,8 +91,10 @@ static inline CTransactionRef make_tx(const std::vector<COutPoint>& inputs, int3
}

/
...

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848502466)
nit: Could emplace movable `CTransaction`, apologies for not including it in my original suggestion.
```suggestion
txs.emplace_back([&] {
```

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1849177866)
nit: Could add `const` to `num_out` for symmetry with `num_in` as you are touching this line (pointed out in https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832788903).

🤔 jonatack reviewed a pull request: "doc: Correct PR Review Club frequency from weekly to monthly"
(https://github.com/bitcoin/bitcoin/pull/31327#pullrequestreview-2447051578)
Perhaps drop the periodicity altogether, as it may evolve.

🤔 tdb3 reviewed a pull request: "doc: Correct PR Review Club frequency from weekly to monthly"
(https://github.com/bitcoin/bitcoin/pull/31327#pullrequestreview-2447134096)
Concept ACK
I agree with the suggestion from @jonatack. It'd be good to drop "monthly" and "meeting".

👍 tdb3 approved a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2447143325)
re ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2

re-ran unit tests and benchmarks, and saw that the dirs generated were random.

👋 adamandrews1's pull request is ready for review: "rpc: combinerawtransaction now rejects unmergeable transactions"
(https://github.com/bitcoin/bitcoin/pull/31298)

📝 kevkevinpal opened a pull request: "test: locking -testdatadir when not specified and then deleting lock and dir at end of test"
(https://github.com/bitcoin/bitcoin/pull/31328)
### Description
Motivated by this comment https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1846842029
I wanted to lock the temp directory like we do when we specify the `-testdatadir` param

### Changes
Now when we run `BasicTestingSetup::~BasicTestingSetup()` we unlock the directory no matter what and we always delete unless `-testdatadir` is used

### Testing
- I've validated by both running the fuzz test runner and individual fuzz tests like so
- `FUZZ=utxo_total_supply ./bu
...

💬 kevkevinpal commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1849400734)
>> Probably we might also need to lock the directory like we do for the custom datadir path.

> Maybe a follow-up can do this? The goal of this pull request is to unbreak OSS-Fuzz (not sure if any other deployment is affected) by simply restoring what has been done for all previous releases in the unit tests

Created this PR, let me know if you would prefer any changes or if you think my approach is incorrect!
https://github.com/bitcoin/bitcoin/pull/31328

💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-2487172554)
It seems like, with https://github.com/bitcoin/bitcoin/pull/31249, I should migrate these test changes as well. WIP

💬 kevkevinpal commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#issuecomment-2487182033)
reACK https://github.com/bitcoin/bitcoin/commit/faaaf59f71ede057b2c1d369ef8db973c2f2dbc2

Ran fuzz tests with 8 jobs and 8 workers `FUZZ=utxo_total_supply ./build_fuzz/src/test/fuzz/fuzz -jobs=8 -workers=8`
and validated that the temp directorys at `/tmp/test_common bitcoin/<random value>` got created and deleted for each fuzz test.

💬 kevkevinpal commented on pull request "doc: Correct PR Review Club frequency from weekly to monthly":
(https://github.com/bitcoin/bitcoin/pull/31327#issuecomment-2487192730)
Concept ACK

I agree with tbd3 and jonatack, drop the "monthly" and "meeting" and even if the frequency changes we won't need to touch this again. Unless the link is dead

🤔 tdb3 reviewed a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2447212379)
Concept ACK

Using `optional` seems much nicer than using a magic value. Planning to circle back to take another look.

⚠️ Klzklzklz opened an issue: "[{Tatra Bank Xvery}]</>"
(https://github.com/bitcoin/bitcoin/issues/31329)
<h1>TATRSKBX</h1>Xx (())<h2>

_[sell&withdraw]</h2>TME 05:52(BTCs)_<5>;
(/wallet@ID:klevismetani@NWO949.microsoft.com~Transferieren_dinero)atach€€@

4405 7783 4587 1093 /fin 09/29 178
1910&@plustomorrow/affecion_tired/sad/dissapoint/dontcry€
[ASAP]()_

✅ achow101 closed an issue: "."
(https://github.com/bitcoin/bitcoin/issues/31329)