✅ pinheadmz closed a pull request: "Create Wap"
(https://github.com/bitcoin/bitcoin/pull/30521)

🤔 tdb3 reviewed a pull request: "rpc: add utxo's blockhash and number of confirmations to scantxoutset output"
(https://github.com/bitcoin/bitcoin/pull/30515#pullrequestreview-2197995886)
Approach ACK

Nice addition. Seems to work well.

Ran a manual test where 102 blocks were generated, a spend to bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5 was done, and two more blocks were generated. The scan height was 104, the height of the UTXO was 103, and it's reported that there are two confirmations.

```
$ src/bitcoin-cli scantxoutset start '["addr(bcrt1qrm87u5l6f455x5lk5f9lmtxahdf39zrk22m8k5)"]'
{
"success": true,
"txouts": 105,
"height": 104,
"bestblock": "4534
...

💬 tdb3 commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690604737)
nitty nit: If you happen to change this file again, could adjust the wording to `The block height at which the scan was done`

💬 tdb3 commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690677294)
It would be great to add tests for these in `test/functional/rpc_scantxoutset.py`.
Maybe something like:
```diff
diff --git a/test/functional/rpc_scantxoutset.py b/test/functional/rpc_scantxoutset.py
index 078a24d577..8edc6d265a 100755
--- a/test/functional/rpc_scantxoutset.py
+++ b/test/functional/rpc_scantxoutset.py
@@ -120,6 +120,11 @@ class ScantxoutsetTest(BitcoinTestFramework):
assert_equal(self.nodes[0].scantxoutset("status"), None)
assert_equal(self.nodes[0].s
...

💬 tdb3 commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690615620)
nit: If the file is touched again.

```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index e964a65a05..5df8641f6d 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2275,7 +2275,7 @@ static RPCHelpMan scantxoutset()
int64_t count = 0;
std::unique_ptr<CCoinsViewCursor> pcursor;
const CBlockIndex* tip;
- CChain* active_chain = nullptr;
+ CChain* active_chain{nullptr};
NodeContext& node = EnsureAny
...

💬 luisschwab commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#issuecomment-2249255752)
Fixed nits and added the test

💬 sipa commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690720835)
I don't think this is safe. When this scope closes, `cs_main` is released, and after that point, `active_chain` points to `CChain` which may be concurrently modified by another thread (which on itself would be undefined behavior already). Worse, it's possible that a reorg occurs (however unlikely) which shortens the chain, and at that point, the `(*active_chain)[coin.nHeight]` call below would be accessing the `CChain::vChain` vector out of bounds.

But I don't think you need to. `tip` already
...

💬 andrewtoth commented on pull request "optimization: Precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-2249287267)
Based on your benchmark results, I don't think this can be called an optimization. It seems to be worse for one benchmark, better in another, and roughly the same for the rest. The description also notes that this will not be noticeable by users. In light of that, does the motivation for this PR still hold?

💬 luisschwab commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690734288)
You're right, it is unsafe. Your solution is a lot cleaner as well.

💬 fjahr commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2249392193)
> Can you clarify what you mean with "not quite"? Locally, if I compile with undefined,integer,float-divide-by-zero and run my diff with: UBSAN_OPTIONS="suppressions=$PWD/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/feature_assumeutxo.py, I get the same crash.

Ah, it wasn't clear to me that the I would have to run it with the sanitizer. That's why it didn't fail for me.

You can reproduce the issue and make the test fail with t
...

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690825056)
> while we're at it, should I also remove this cast?

Sure, the cast is completely harmless (because a value high enough for the sign or value to change can't exist), but the cast is confusing at best, so best to remove.

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#issuecomment-2249457002)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690827566)
nit: The two fields should have the same wording, as they refer to the same block

💬 maflcko commented on pull request "rpc: add utxo's blockhash and number of confirmations to scantxoutset output":
(https://github.com/bitcoin/bitcoin/pull/30515#discussion_r1690834408)
```suggestion
{RPCResult::Type::NUM, "confirmations", "Number of confirmations of the unspent transaction output when the scan was done"},
```

nit: (Same wording here, because the `confirmations` refers in the calculation to the same block)

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690844876)
> ```diff
> + COutPoint out{*txid, static_cast<uint32_t>(nOut)};
> ```

Thanks for looking again. For reference, `getInt` has a built in check, so you could use `getInt<uint32_t>` and avoid this cast. That'd be a behavior change for out-of-range values, but the values are out of range anyway, so should be fine.

📝 fanquake locked a pull request: "Create Wap"
(https://github.com/bitcoin/bitcoin/pull/30521)
http://waptrick.com

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements o
...

✅ paplorinc closed a pull request: "optimization: Precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher"
(https://github.com/bitcoin/bitcoin/pull/30442)

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1690941661)
Hmm https://corecheck.dev/bitcoin/bitcoin/pulls/30454 is down again

👍 BrandonOdiwuor approved a pull request: "Rendering an amp characters in the wallet name for QMenu"
(https://github.com/bitcoin-core/gui/pull/828#pullrequestreview-2198547338)
ACK 8233ee41ab9648cd0c3bd78bc2a8d692a54d9ea0. Tested on Ubuntu 22.04 using Qt version 5.15.3

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1690990701)
The rename could be split out and merged ahead?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1690979665)
note to myself: Could split this up