🤔 josibake reviewed a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1772112219)
ACK https://github.com/bitcoin/bitcoin/pull/25273/commits/1d1a6ff03215cf3aadd5fd1118f354b1ba8ffa5a

Left some nits that can be ignored or addressed in a follow-up, but nothing major. There is a type mismatch with `GetInputWeight` returning int64_t but being assigned to int32_t. Would be great to fix that but also not a super big deal since weight will never be greater than int32_t anyway (but should def be fixed in a followup).

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420275230)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: kind of annoying that we call this an `output` but below in `GetExternalOutput` we (correctly) call it an `outpoint`, would be nice to use consistent naming if this ever gets retouched.

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420279769)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: `s/output/outpoint/`

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420316907)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

`GetInputWeight` is currently returning `int64_t` but I think `int32_t` is the more appropriate choice given that this represents weight.

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420318483)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

same as above, type mismatch between `int32_t` and what's returned, `int64_t`

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420293648)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: `s/out/outpoints/`, in this case I think it helps make it more clear why you are doing the the transform: we are returning just the outpoints from the `m_selected` vector

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420333328)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

`int32_t`

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420306769)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

Is there a situation where an internal input would have a value for `TxOut`, accidentally or purposefully? Only asking because it feels _slightly_ more fragile to rely only on this to determine if something is external or internal vs having two separate vectors, but also don't have a good sense if what I'm worried about is even possible (internal having `TxOut` set) or if its even that bad if we
...

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420348354)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/2a06c4c2b33c614cc3f877bdc013c8df9e847ca9:

in earlier commits, you were replacing the "Has -> Get" pattern with a "Get" that returned a std::optional. Why can't we also do that here?

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420332476)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

shouldn't this be `int32_t`?

💬 fanquake commented on issue "bitcoin 25.1 regression test failure against sqlite 3.44.1":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1847113188)
This is happening again in https://github.com/Homebrew/homebrew-core/pull/156745.
i.e https://github.com/Homebrew/homebrew-core/actions/runs/7141174823/job/19448089734?pr=156745#step:4:47.

💬 0xB10C commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847121311)
post-merge ACK. Tested this on my NixOS setup. Configure and build works fine, unit tests pass. Reverting in faa48388bca06df1ca7ab92461b76a6720481e45 makes sense.



<details>
<summary>
I see a few `-Wmaybe-uninitialized` warnings not sure if they're related.
</summary>

```
CXX policy/libbitcoin_common_a-feerate.o
In file included from ./hash.h:13,
from ./pubkey.h:10,
from ./addresstype.h:9,
from ./outputtype.h:9,

...

💬 dergoegge commented on issue "fuzz: Left over tmp files when fuzzing with afl++":
(https://github.com/bitcoin/bitcoin/issues/28811#issuecomment-1847123265)
* we could (but probably won't) refactor our code so that we don't need to go to disk at all.
* or use a custom signal handler for e.g. SIGUSR1 that deletes the data dir and then exits without any other cleanup. Afl++ can be told to send any signal instead of SIGKILL to children on timeout, etc with `AFL_KILL_SIGNAL`.

I've worked around this so far by using a ram disk and fuzzing inside a docker container, once the disk is full the whole thing just dies and I can inspect the container afterw
...

🤔 BrandonOdiwuor reviewed a pull request: "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs"
(https://github.com/bitcoin/bitcoin/pull/28979#pullrequestreview-1772293975)
Concept ACK

⚠️ maflcko reopened an issue: "bitcoin 25.1 regression test failure against sqlite 3.44.1"
(https://github.com/bitcoin/bitcoin/issues/28941)
While upgrading sqlite to 3.44.1, we've found some regression test failure


<details>
<summary>test failure log</summary>

```
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/25.1/bin/test_bitcoin
Running 557 test cases...
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [3933824172291540 != 0]
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1522707584769707 != 0]
t
...

💬 maflcko commented on issue "bitcoin 25.1 regression test failure against sqlite 3.44.1":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1847146801)
Again, it would be good to have steps to reproduce locally.

Also, `double_serfloat_tests` has nothing to do with sqlite. (You can compile them without wallet or sqlite)

📝 dergoegge opened a pull request: "fuzz: Improve fuzzing stability for txorphan harness"
(https://github.com/bitcoin/bitcoin/pull/29031)
The `txorphan` harness has low stability as eviction of orphan txs is entirely random at the moment.

Fix this by passing the rng to `LimitOrphans`, which can be deterministic in tests.

Also see #29018.

👋 dergoegge's pull request is ready for review: "fuzz: Improve fuzzing stability for txorphan harness"
(https://github.com/bitcoin/bitcoin/pull/29031)

💬 brunoerg commented on pull request "fuzz: Improve fuzzing stability for txorphan harness":
(https://github.com/bitcoin/bitcoin/pull/29031#issuecomment-1847155802)
Concept ACK

nice!

💬 furszy commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1420492611)
no, it doesn't. Initially, the `wallet_path` was inside the `bench.run()` lambda to create wallets under different paths. This was to avoid flushing the wallet to disk and removing the directory within the benchmark execution (which has nothing to do with what we are trying to benchmark). Then, at the end, I changed the approach to occupy less memory.

But.. thinking it further now, we could go back to the previous approach and not be concerned about memory by setting the max iterations number
...