💬 kevkevinpal commented on pull request "wallet: Add tracing for sqlite statements":
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1572808184)
ACK [f45e066](https://github.com/bitcoin/bitcoin/pull/27801/commits/f45e0669dd1f1bdb243e645691e44993905a3919) I tested by switching to this commit and doing the following
```
make -j"$(($(nproc)+1))"
./src/bitcoind -regtest -debug=walletdb -loglevel=walletdb:trace
```
In different window
```
./src/bitcoin-cli -regtest createwallet "node2"
```

Then you can observe the bitcoind logs
example of one
```
2023-06-01T21:14:59Z [/Users/<my path>/wallet.dat] SQLite Statement: INSERT or REP
...

🤔 furszy reviewed a pull request: "streams: Drop confusing DataStream::Serialize method and << operator"
(https://github.com/bitcoin/bitcoin/pull/27800#pullrequestreview-1456346592)
lgtm ACK 5cd0717a

💬 ryanofsky commented on pull request "streams: Drop confusing DataStream::Serialize method and << operator":
(https://github.com/bitcoin/bitcoin/pull/27800#discussion_r1213705810)
> does this need to be a Span? Wouldn't be the same to just pass `finalAlert`?

Because `finalAlert` is a vector of bytes, serializing the vector would first write the number of bytes in the vector to the stream, followed by the bytes themselves. Since spans unlike vectors are fixed length, serializing a span just writes the raw bytes to the stream without a length prefix.

💬 achow101 commented on pull request "[24.x] rpc: Fix invalid bech32 handling":
(https://github.com/bitcoin/bitcoin/pull/27755#issuecomment-1572821465)
ACK c2e9214effe9abecae6f81cb10158f9661065da3

Verified that the additional changes are the minimal required to make the new test work.

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213710664)
Done

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1572825952)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213485109 and https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213479178

🚀 achow101 merged a pull request: "[24.x] rpc: Fix invalid bech32 handling"
(https://github.com/bitcoin/bitcoin/pull/27755)

💬 achow101 commented on pull request "[23.x] rpc: Fix invalid bech32 handling":
(https://github.com/bitcoin/bitcoin/pull/27756#issuecomment-1572828786)
ACK d98770720885cdce1bbe2109a6d214c63fa1814a

Diff matches #27755

🚀 achow101 merged a pull request: "[23.x] rpc: Fix invalid bech32 handling"
(https://github.com/bitcoin/bitcoin/pull/27756)

💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1572835701)
> Is there really a qualitative difference between the errors?

I didn't dig into the actual errors, but the qualitative difference here seems to be that the other AbortNode errors actually need to be fatal and should try to shut down to prevent getting into a worse state. But an error flushing state in memory to disk does not need to be fatal, as long the state in memory is kept and can be saved later. There are lots of ways an application could handle a failed flush notification, and I think
...

💬 furszy commented on pull request "streams: Drop confusing DataStream::Serialize method and << operator":
(https://github.com/bitcoin/bitcoin/pull/27800#discussion_r1213724380)
ah ok. I mixed up stuff by reading the `CVectorWriter` class description -> "Minimal stream for overwriting and/or appending to an existing byte vector" (it doesn't mention the serialization..). I should have checked the constructor. My bad, thanks.

💬 furszy commented on pull request "Introduce 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1572850482)
title and description updated.
And also went a bit further with the `rpc_getblockfrompeer.py` changes, added some explanations to it.

📝 Xekyo opened a pull request: "[Fuzz/Bugfix] Mitigate timeout in CalculateTotalBumpFees"
(https://github.com/bitcoin/bitcoin/pull/27803)
The slow fuzz seed described in #27799 was just slower than expected, not an endless loop. Ensuring that every anscestor is only processed once speeds up the termination of the graph traversal.

Fixes #27799

💬 Xekyo commented on issue "fuzz: mini_miner: Timeout in mini_miner":
(https://github.com/bitcoin/bitcoin/issues/27799#issuecomment-1572852886)
Letting the timeout fuzz seed run longer results in it terminating eventually. I propose a fix in #27803

💬 denavila commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1572869964)
> See for example https://github.com/achow101/wallet-fingerprinting/blob/main/fingerprints.md

Thanks!
At a quick glance, it looks like Core's transactions are somewhat close to Electrum's.
I'm not familiar with some of the items, but assuming it's possible, I can look into inject some variety in the "deniabilized" transactions to make them resemble a variety of wallets (eg Electrum as a start).
Is there prior work in this by any chance? That would help me for sure.
Also, should I pursu
...

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1213739677)
Yes, I agree with you. stale estimates should not be read. I have updated the MAX_FILE_AGE to 60 hours, which 2.5 days, as @instagibbs suggested that longer would be okay?

💬 denavila commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1572892485)
> It doesn't improve privacy.

Can you please elaborate why you think so?
Paul's blog post had some pretty compelling arguments why this scheme should work and I think it has a lot of merit.

> False sense of privacy implemented in this pull request only helps miners with fees.

It's important to point out that this PR doesn't add functionality that couldn't be achieved manually before.
If a user wanted to split a coin and send it to themselves, they could do it with the existing Bitcoi
...

💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1213768416)
> Just for clarity: This would imply passing a reference of the kernel context to the validation and blockstorage code? Giving the kernel ownership over this interrupt state seems a bit weird to me at first glance, since it is shared with all the other components that we have.

I think the idea is to give the kernel ownership of it's own interrupt state, and not have to rely on an external mechanism. If other components that are already depending on the kernel want to share the same interrupt
...

👍 theStack approved a pull request: "wallet: Add tracing for sqlite statements"
(https://github.com/bitcoin/bitcoin/pull/27801#pullrequestreview-1456464007)
tACK f45e0669dd1f1bdb243e645691e44993905a3919

Tested on OpenBSD 7.3, sqlite 3.41.0. Started bitcoind with `debug=walletdb -loglevel=walletdb:trace`, ran RPCs `createwallet`, `getnewaddress` and `setlabel` and observed the debug output emiting sqlite statements (`SQLite Statement: INSERT or REPLACE into main values(.....)`.

💬 furszy commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213799284)
This isn't covered by the tests.

In the test, all prefixes are serialized into a `DataStream` and then provided to `GetNewPrefixCursor`, so the size is always part of the data. Therefore, we never get up to this point.

Still, it doesn't seems to be reachable right now; As the db handler write function always serializes data, the size is always there.

But.. for the sake of test coverage completeness and leave nothing to chance, made a commit that exercises it: https://github.com/furszy/b
...