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

💬 maflcko commented on pull request "fuzz: Improve fuzzing stability for txorphan harness":
(https://github.com/bitcoin/bitcoin/pull/29031#issuecomment-1847221427)
lgtm ACK 15f5a0d0c8ce6b306cdeba6a4777334b848a76aa

Didn't test the stability score.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1847225184)
Rebased with https://github.com/bitcoin/bitcoin/pull/28349 merged.

> While this is rebased and ready for review again, I'd prefer progress on https://github.com/bitcoin/bitcoin/pull/25832 while we're exploring alternatives to https://github.com/bitcoin/bitcoin/pull/26593. I'll leave this as work-in-progress until we're clearer on alternatives.

I've also been working on a branch that adds tracepoint support for macOS and FreeBSD based on this branch. Doesn't need dtrace and doesn't need any
...

💬 dergoegge commented on pull request "fuzz: Improve fuzzing stability for txorphan harness":
(https://github.com/bitcoin/bitcoin/pull/29031#issuecomment-1847231728)
> Didn't test the stability score.

I'm seeing 87% stability locally, up from 51% reported by oss-fuzz. Not sure why it isn't a 100%, can't see anything else in the orphanage that would be non-deterministic.