💬 macgyver13 commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2234032395)
This is the format I used to import scan and spend private keys:
sp([fingerprint/352'/1'/0'/1'/0]WIF(private scan key),[fingerprint/352'/1'/0'/0'/0]WIF(private spend key)#checksum

🤔 cedwies reviewed a pull request: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/33064#pullrequestreview-3059599768)
Tested on MacOS 15.5 (Debug build).

- unit tests pass (0/143 failures)
- ./test/functional/wallet_transactiontime_rescan.py passes (11 s)

Question: would it make sense to also test "abortrescan" **during** an active rescan to hit the True path and ensure the scan halts as expected?

ACK 8aed477

💬 yuvicc commented on pull request "test: refactor mempool_accept_wtxid":
(https://github.com/bitcoin/bitcoin/pull/33067#issuecomment-3124693888)
Concep ACK
This would make the test consistent with `mempool_accept`

💬 goodgollyholly commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2234123982)
Hi need help my laptop was stollen and all i have is my cell phone from like the ice age do i need to download my core wallet to execute this

💬 ariard commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3124747814)
> but BIP editors have been highly resistant to it, hence doc/policy.

I think it’s def worthy to have BIP even for tx-relay policy once it has spent the test of time (a la BIP125 for replacement mechanism), though here it’s a bastard situation of a policy rule that is intended to be consensus sooner or latter, and which is already documented in its own BIP. But during the meanwhile, there can be downside effects on multi-party tx flows.

It’s not clear if it’s worthy to document in doc/poli
...

💬 jonatack commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3124756789)
> I think there are a lot of cases where a document would be helpful but BIP editors have been highly resistant to it, hence doc/policy.

More recently, BIPs involving policy R&D and interoperability have been given BIP numbers and merged. Perhaps propose on the ML for community feedback.

💬 Zeegaths commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3124757121)
I ran these commands on Ubuntu 22:

sudo ip addr add 1.1.1.1/32 dev wlo1
sudo ip addr add 2.2.2.2/32 dev wlo1

./build/bin/bitcoind -regtest -bind=0.0.0.0:18444 -discover -daemon
./build/bin/bitcoin-cli -regtest getnetworkinfo | grep -A 10 "localaddresses"

**Result on master:**
"localaddresses": [
],
"warnings": [
"This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
]


**Result on PR:**
"localaddresses": [
{
...

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2234167309)
done, thanks

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2234167334)
done

🤔 jonatack reviewed a pull request: "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline"
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3059739878)
Review ACK d5104cfbaeb82081e4b00a5084516555e446dcdc

💬 jonatack commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2234192567)
I don't mind re-reviewing if you take that approach.

📝 b-l-u-e opened a pull request: "[p2p] Fix signed integer overflow in LocalServiceInfo::nScore"
(https://github.com/bitcoin/bitcoin/pull/33072)
## Problem
The `nScore` field in `LocalServiceInfo` struct was defined as `int` (32-bit signed integer), which could overflow from `INT_MAX` (2,147,483,647) to `INT_MIN` (-2,147,483,648) when incremented by `SeenLocal()` during version handshakes. This is undefined behavior in C++ and could affect address selection logic.

## Solution
Change `nScore` from `int` to `int64_t` (64-bit signed integer) to provide a much larger range and prevent overflow in realistic scenarios.

## Changes
- **
...

💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3124944751)
ACK 943ee6768d9f94da70aaf90d346d4cf2ea1fa39d

Thanks for adding and tweaking the test (checked, it's still passing locally as well)

💬 ajtowns commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-3125312465)
ACK 1c10b7351e194fc788766347f65f4512f61f05e8

💬 gmaxwell commented on pull request "[p2p] Fix signed integer overflow in LocalServiceInfo::nScore":
(https://github.com/bitcoin/bitcoin/pull/33072#issuecomment-3125357027)
This doesn't compile, and why not just saturate? e.g. if(nScore < std::max<int>)nScore++

"No performance impact (64-bit integers are efficient on modern systems)" sounds like LLM blather, the relevant criteria is not the performance of 64-bit integers considering how little it's used. Memory usage might matter if this was a per-peer (or per-block/header) field, but it isn't.

The purpose of it is to track input addresses to see that they were, in fact, reachable. I'm pretty sure that by
...

💬 maflcko commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r2234919472)
I don't think this is accurate. The limit is no longer per output (or possibly per mempool). The limit is now over the total size of all datacarrier output scripts in a single transaction in the mempool.

💬 maflcko commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r2234905470)
nit: Seems a bit odd to use `0` and `nullopt` interchangeably to mean the same (and require call sites to do the conversion manually). It would be easier to just flatten the optional and have a single dimension, directly translating two-way to the `-datacarriersize` option.

Diff:

```diff
diff --git a/src/kernel/mempool_options.h b/src/kernel/mempool_options.h
index d57dbb393f..cf873dd48d 100644
--- a/src/kernel/mempool_options.h
+++ b/src/kernel/mempool_options.h
@@ -48,9 +48,8 @@ struct MemPo
...

💬 maflcko commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-3125846012)
> test_framework.authproxy.JSONRPCException: 'rescanblockchain' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)

You may want to try to increase the test_runner's timeout-factor locally. Increasing the timeout of this specific RPC call is also possible, but leads to a whack-a-mole in the long run.

💬 ajtowns commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r2235004536)
FWIW, that cleanup doesn't seem worthwhile for an option that's already deprecated to me

💬 ajtowns commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r2235006754)
It already uses the plural "outputs" though?