fix: add regtest hrp for segwit bitcoin address

[fdefelici]

Description

This PR allows BitcoinAddress to manage segwit addresses for regtest (hrp = "bcrt").

The following apis, now works nicely with regtest hrp:

• SegwitBitcoinAddress::from_bech32(..)
• BitcoinAddress::from_string(..)

Applicable issues

• fixes Allow BitcoinAddress to represent addresses with Regtest HRP prefix #6366

Additional info (benefits, drawbacks, caveats)

Convert from Option to Result?

LegacyBitcoinAddress::from_b58(str) already use Result.
Possibly, we could then propagate Result to Address::from_string()

stacks-core/stackslib/src/burnchains/bitcoin/address.rs

Lines 302 to 307 in 9d53951

	pub fn from_bech32(s: &str) -> Option<SegwitBitcoinAddress> {
	let (hrp, quintets, variant) = bech32::decode(s)
	.inspect_err(|_e| {
	test_debug!("Failed to decode '{s}': {_e:?}");
	})
	.ok()?;

PoxAddress related

Current patch is applying a fallback that convert back and forth: is_mainnet (bool) <=> NetworkType

stacks-core/stackslib/src/chainstate/stacks/address.rs

Lines 379 to 399 in 9d53951

	pub fn to_b58(self) -> String {
	match self {
	PoxAddress::Standard(addr, _) => addr.to_b58(),
	PoxAddress::Addr20(mainnet, addrtype, addrbytes) => match addrtype {
	PoxAddressType20::P2WPKH => {
	let btc_addr = SegwitBitcoinAddress::P2WPKH(mainnet, addrbytes);
	btc_addr.to_bech32()
	}
	},
	PoxAddress::Addr32(mainnet, addrtype, addrbytes) => match addrtype {
	PoxAddressType32::P2WSH => {
	let btc_addr = SegwitBitcoinAddress::P2WSH(mainnet, addrbytes);
	btc_addr.to_bech32()
	}
	PoxAddressType32::P2TR => {
	let btc_addr = SegwitBitcoinAddress::P2TR(mainnet, addrbytes);
	btc_addr.to_bech32()
	}
	},
	}
	}

stacks-core/stackslib/src/chainstate/stacks/address.rs

Lines 443 to 460 in 9d53951

	pub fn try_from_bitcoin_output(o: &BitcoinTxOutput) -> Option<PoxAddress> {
	match &o.address {
	BitcoinAddress::Legacy(ref legacy_addr) => {
	let addr = StacksAddress::from_legacy_bitcoin_address(legacy_addr);
	let pox_addr = PoxAddress::Standard(addr, None);
	Some(pox_addr)
	}
	BitcoinAddress::Segwit(SegwitBitcoinAddress::P2WPKH(is_mainnet, bytes_20)) => Some(
	PoxAddress::Addr20(*is_mainnet, PoxAddressType20::P2WPKH, *bytes_20),
	),
	BitcoinAddress::Segwit(SegwitBitcoinAddress::P2WSH(is_mainnet, bytes_32)) => Some(
	PoxAddress::Addr32(*is_mainnet, PoxAddressType32::P2WSH, *bytes_32),
	),
	BitcoinAddress::Segwit(SegwitBitcoinAddress::P2TR(is_mainnet, bytes_32)) => Some(
	PoxAddress::Addr32(*is_mainnet, PoxAddressType32::P2TR, *bytes_32),
	),
	}
	}

Checklist

• Test coverage for new or modified code paths
• Changelog is updated
• Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
• New clarity functions have corresponding PR in clarity-benchmarking repo
• New integration test(s) added to bitcoin-tests.yml

[jcnelson]

I think this is fine for now.

LegacyBitcoinAddress::from_b58(str) already use Result.
Possibly, we could then propagate Result to Address::from_string()

The reason we don't is because there's not much the client can do based on the decoding error. A string either decodes to a LegacyBitcoinAddress, or it doesn't. If it doesn't, then the fundamental reason is that the string is incorrect.

In curreny patch applying a fallback that convert back and forth: is_mainnet (bool) <=> NetworkType

That's okay. For now, the focus is on getting regtest HRP support for BitcoinAddress in order to unblock progress on the new Bitcoin RPC client. Considerations for altering PoxAddress can wait until the bitcoin_regtest_controller.rs file is refactored, since there will need to be a way to convert PoxAddress (which the node uses internally to represent Bitcoin addresses that receive PoX payouts) to and from BitcoinAddress.

[fdefelici]

LegacyBitcoinAddress::from_b58(str) already use Result.
Possibly, we could then propagate Result to Address::from_string()

The reason we don't is because there's not much the client can do based on the decoding error. A string either decodes to a LegacyBitcoinAddress, or it doesn't. If it doesn't, then the fundamental reason is that the string is incorrect.

Got it.

I was just asking because it could be useful to have an error message in case deserialization of addresses fails in the new RPC Client. Just In case, I have the patch ready for this (not pushed yet)

That said, I’m also fine with keeping the current implementation as-is.

[jcnelson]

LGTM

[Jiloc]

lgtm!

[codecov]

Codecov Report

❌ Patch coverage is 89.26554% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.23%. Comparing base (a9e2828) to head (76bd51a).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
stackslib/src/burnchains/bitcoin/address.rs	91.83%	12 Missing ⚠️
...ackslib/src/chainstate/burn/operations/test/mod.rs	0.00%	4 Missing ⚠️
stackslib/src/burnchains/bitcoin/bits.rs	25.00%	3 Missing ⚠️

❌ Your project status has failed because the head coverage (76.23%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #6373       +/-   ##
============================================
+ Coverage    62.44%   76.23%   +13.78%     
============================================
  Files          552      552               
  Lines       351797   351925      +128     
============================================
+ Hits        219676   268277    +48601     
+ Misses      132121    83648    -48473     

Files with missing lines	Coverage Δ
stackslib/src/burnchains/bitcoin/mod.rs	38.55% <100.00%> (+7.78%)	⬆️
stackslib/src/chainstate/stacks/address.rs	95.06% <100.00%> (+46.13%)	⬆️
stackslib/src/burnchains/bitcoin/bits.rs	81.07% <25.00%> (+26.50%)	⬆️
...ackslib/src/chainstate/burn/operations/test/mod.rs	0.00% <0.00%> (-72.50%)	⬇️
stackslib/src/burnchains/bitcoin/address.rs	86.01% <91.83%> (+24.25%)	⬆️

... and 379 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9e2828...76bd51a. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.