💬 tdb3 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2033263056)
> Tested ACK. The new error handling code looks good to me upon review. Ran the daemon and gave invalid values for `rpcconnect` and `rpcport` and the proper error messages came up. The behavior of port 0 being invalid is consistent with `src/init.cpp`. Ran `interface_bitcoin_cli.py` too and there were no issues with the added tests.

Thank you for reviewing, @marcofleon!

💬 pablomartin4btc commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#discussion_r1548752403)
**nit**: the tx won't be created when an error is raised during the `createTransaction`'s call (eg no recipient) so the obj should represent the correct thing, unless I misunderstanding the use case here... ok I see now this is how `src/util/result.h` works, so ignore my correction, I find it a bit weird but perhaps cos I'm not used to it yet. As a ref this change could be useful https://github.com/bitcoin/bitcoin/pull/25665
```suggestion
virtual util::Result<CreateTransactionResult> crea
...

👍 pablomartin4btc approved a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-1975255350)
cr ACK dfbd145676255f82bad529976928dff1939c5e48

💬 davidgumberg commented on pull request "lint: Rewrite spelling, includes, and include guards linters in Rust":
(https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2033339070)
I've substantially revised this PR to address @fjahr's feedback that the rust lint runner monofile was getting too big, and that it doesn't support running individual tests (https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2022796657) and @maflcko's suggestion to include `test/lint/lint-locale-dependence.py` and `test/lint/lint-python-utf8-encoding.py` (https://github.com/bitcoin/bitcoin/pull/29744#issuecomment-2022606314).

I do not feel confident in regex, and had to do some tricky
...

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548842239)
nit: not sure if this comment is redundant now since the function name is `calculate_input_weight`

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548842910)
nit: not sure if this comment is redundant now since the function name is `calculate_input_weight`

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548845139)
It might make sense to add another test that does not explicitly set `witness_stack_hex` equal to `None`

```
self.assertEqual(calculate_input_weight(""),
(SKELETON_BYTES + SMALL_LEN_BYTES) * WITNESS_SCALE_FACTOR)
```

💬 kevkevinpal commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1548863105)
nit: we only need to define this once

💬 pablomartin4btc commented on pull request "Fix create unsigned transaction fee bump":
(https://github.com/bitcoin-core/gui/pull/812#discussion_r1548912316)
At least on Ubuntu 22.04, now after the fee-bumping unsigned a modal pop-up is displayed:


while before there was a message sent thru the OS system information (?):


I see in the code we usually use `CClientUIInterface::MSG_INFORMATION` (`transactionview.cpp`, `walletview.cpp`) and I'd pre
...

👍 pablomartin4btc approved a pull request: "Fix create unsigned transaction fee bump"
(https://github.com/bitcoin-core/gui/pull/812#pullrequestreview-1975499213)
tACK 671b7a32516d62e1e79393ded4b45910bd08010a

Tested and verified this PR fixes #810 and now creation of unsigned tx/ PSBT/ during fee-bumping doesn't ask for passphrase/ requires wallet unlock.

👍 pablomartin4btc approved a pull request: "Change example address from legacy (P2PKH) to bech32m (P2TR)"
(https://github.com/bitcoin-core/gui/pull/808#pullrequestreview-1975550497)
tACK c6d1b8de89d87fe4fd171dc85557299e429e6564

<details>
<summary>Tested running <code>bitcoin-qt</code> on <code>signet</code>.</summary>


</details>

<details>
<summary>Also tested the script provided in the <a href="https://github.com/bitcoin-core/gui/pull/808#issue-2202921882">description</a>.</summary>

```
/test/functional$ ./gen_add_type.py
mainnet example address: bc1p35yvje
...

💬 0xB10C commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2033694468)
I'm missing context for this change. Why no PR description and no commit message body?

⚠️ pinwhell opened an issue: "Concern: Transatlantic Internet Seizure, Impact on Bitcoin Transactions & Blockchain Forks"
(https://github.com/bitcoin/bitcoin/issues/29795)
How could a prolonged transatlantic internet disruption, such as one triggered by war or sabotage, impact Bitcoin transactions and user funds? Specifically, what might occur in terms of a potential blockchain split or fork, and how could efforts to reconcile such a situation be affected?

✅ maflcko closed an issue: "Concern: Transatlantic Internet Seizure, Impact on Bitcoin Transactions & Blockchain Forks"
(https://github.com/bitcoin/bitcoin/issues/29795)

💬 maflcko commented on issue "Concern: Transatlantic Internet Seizure, Impact on Bitcoin Transactions & Blockchain Forks":
(https://github.com/bitcoin/bitcoin/issues/29795#issuecomment-2033768668)
Usually the issue tracker is used to track technical issues related to the Bitcoin Core code base.

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat, or one of the Bitcoin subreddits, or any other place that you feel is well suited.

Network-wide consensus and/or P2P changes first need to be discussed with the greater community, for example https://groups.google.c
...

💬 Sjors commented on pull request "doc: Update the developer mailing list address.":
(https://github.com/bitcoin/bitcoin/pull/29782#issuecomment-2033776628)
ACK 0ead466a0c72bef0a8622749b84e9c7c5c37144f

💬 Sjors commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2033781705)
> is the desire here to have the unit tests run by default when running individual tests

No

> (and also by default when running all tests)?

Yes

They are just another test now, so they don't need special treatment via `--skipunit`.

> The user can specify --failfast to end testing early if any test fails (unit tests are no exception)

Yes, that seems good enough.

💬 maflcko commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2033789710)
> It seems unwise to have CI that runs for every PR to be dependent on an unstable OS. From what I can tell, the purpose of using Ubuntu noble is to get access to clang 18. Instead of doing that, could we instead use the LTS release and just use the [llvm package repo](https://apt.llvm.org/) to get the clang versions we want?

I agree, but that is not possible, because the bpfcc-tools on the Ubuntu 22.04 LTS were outdated as well. So they'd require a ppa as well, see https://github.com/bitcoin
...

💬 maflcko commented on pull request "ci: Temporarily disable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/pull/29788#issuecomment-2033790711)
> I'm missing context for this change. Why no PR description and no commit message body?

I put it in the first comment. Edited and moved to the description.

💬 Sjors commented on pull request "rpc: Optimize serialization disk space of dumptxoutset - Reloaded":
(https://github.com/bitcoin/bitcoin/pull/29612#issuecomment-2033794015)
> I think it would be better if the new format could include a version number so that this could be properly reported.

This seems useful in general.

> We could detect which format was used for serialization and proceed to deserialize as necessary.

Yes, but I don't think we need to support the format from before this PR - once we have the mainnet params, the instruction could simply be to use v28.0 or newer to generate the snapshot.