💬 furszy commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#discussion_r1237708053)
This loop over all entries breaks the purpose of using a map, could have been a set<pair<string, method>>.

Or.. instead of having the loop, could:
```c++
std::string method = parsed_command[0];
bool exec_help = false;
if (method == "help") {
exec_help = true;
method = parsed_command[1];
}
auto it_method = m_method_map.find(method);
if (it_method == m_method_map.end()) return false; // method not found
return (*it_method.second)(parsed_command, wallet_model, exec_help);
```

💬 Sjors commented on pull request "Drop macOS ForceActivation workaround":
(https://github.com/bitcoin-core/gui/pull/744#issuecomment-1618472088)
Tested 362e98937356a74a5b70d65348957edf05b1138a but it seems the workaround is still needed.

This was introduced in https://github.com/bitcoin/bitcoin/pull/14123 as a fix to https://github.com/bitcoin/bitcoin/issues/13829.

With this PR, built on macOS 13.4.1 (Intel) with qt@5/5.15.10 via Homebrew, when I use `⌘ + H` to hide the window, and then e.g. click on "Receive" in the dock, the window doesn't appear. Without this PR the window _does_ appear.

💬 john-moffett commented on pull request "Exit and show error if unrecognized command line args are present":
(https://github.com/bitcoin-core/gui/pull/742#issuecomment-1618487197)
I could modify this PR to enforce that behavior and add the documentation to `bitcoin-qt -help`. Right now, the PR only allows non-option arguments which must start with `bitcoin:` (case insensitive). It will still ignore any options after that.

I think leaving the current `master` behavior and just adding documentation to reflect it would be insufficient. My worry is that a user could be fooled by something like:

Scammer: *"Can you send me some testnet coins? Start your client with `bitco
...

💬 hebasto commented on pull request "Fix transaction view/table":
(https://github.com/bitcoin-core/gui/pull/662#issuecomment-1618516641)
Tested 8e428cc1188568aaeeaeb8f1f4498ae5b738a5e2 on Ubuntu 23.04.

Application window resizing leads to:


when the content of the last column "Amount" goes out of the scope.

🤔 furszy reviewed a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692#pullrequestreview-1511346984)
Two small scenarios to fix:

1) Should guard `generate` from using negative numbers. Currently, can call `generate -20` and the process will return an empty blocks array instead of failing.
2) The `generatetoaddress` result has a bracket without the correct indentation. See
<img width="549" alt="screenshot" src="https://github.com/bitcoin-core/gui/assets/5377650/d1459746-0732-4dd2-90b4-d2403704677c">

💬 hebasto commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#issuecomment-1618528575)
ping @john-moffett :)

💬 ItIsOHM commented on issue "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1618551972)
Hello @stickies-v, I added these two entries after `rules` in `template_request`:

Is this correct? I'm sorry if it's wrong, I'm just a beginner with this.

💬 MarcoFalke commented on pull request "ci: Remove deprecated container.greedy":
(https://github.com/bitcoin/bitcoin/pull/28024#issuecomment-1618569388)
> It is still mentioned

I asked by email.

💬 MarcoFalke commented on pull request "ci: Remove deprecated container.greedy":
(https://github.com/bitcoin/bitcoin/pull/28024#issuecomment-1618572545)
Added an unrelated commit to adjust the CCACHE_SIZE. For some reason the jammy macOS taks is uncacheable? https://cirrus-ci.com/task/4744875699077120?logs=ci#L2486

💬 hebasto commented on pull request "ci: Remove deprecated container.greedy":
(https://github.com/bitcoin/bitcoin/pull/28024#issuecomment-1618585164)
> For some reason the jammy macOS taks is uncacheable? [cirrus-ci.com/task/4744875699077120?logs=ci#L2486](https://cirrus-ci.com/task/4744875699077120?logs=ci#L2486)

See:
- https://github.com/bitcoin/bitcoin/issues/21552
- https://github.com/bitcoin/bitcoin/pull/24620

💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1618628124)
Maybe remove the sub-test case?

📝 theStack opened a pull request: "test: refactor: deduplicate legacy ECDSA signing for tx inputs"
(https://github.com/bitcoin/bitcoin/pull/28025)
There are several instances in functional tests and the framework (MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy ECDSA signature for a certain transaction's input by doing the following steps:

1. calculate the `LegacySignatureHash` with the desired sighash type
2. create the actual digital signature by calling `ECKey.sign_ecdsa` on the signature message hash calculated above
3. put the DER-encoded result as CScript data push into tx input's scriptSig

Create a new
...

💬 john-moffett commented on pull request "Switch RPCConsole wallet selection to the one most recently opened/restored/created":
(https://github.com/bitcoin-core/gui/pull/696#issuecomment-1618803949)
Thanks for pings, @hebasto and @jarolrod. Apologies for the delay. I've renamed the method and moved the declaration to the `Q_SLOTS` section.

💬 theStack commented on pull request "Remove confusing "Dust" label from coincontrol / sendcoins dialog":
(https://github.com/bitcoin-core/gui/pull/719#issuecomment-1618813452)
> The commit message and the PR description should mention changes in the `SendCoinsDialog` as well.

Sorry for the reply there, missed that. Force-pushed with an adapted commit message (and changed PR title / description as well):
```
$ git range-diff 394300c18...ef4185d2b
1: 944263a93 ! 1: 210ef1e98 qt: remove confusing "Dust" label from coincontrol dialog
@@ Metadata
Author: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

## Commit message ##
- qt: rem
...

💬 theStack commented on pull request "Remove confusing "Dust" label from coincontrol / sendcoins dialog":
(https://github.com/bitcoin-core/gui/pull/719#discussion_r1251087089)
I'm not too experienced with Qt, can anyone elaborate on that question (ping @furszy as author of the second commit)? It seems at least that we already have a couple of instances in master where both Qt::AlignLeading and Qt::AlignLeft are used together:
```
$ git grep AlignLeading
src/qt/forms/debugwindow.ui: <set>Qt::AlignBottom|Qt::AlignLeading|Qt::AlignLeft</set>
src/qt/forms/helpmessagedialog.ui: <set>Qt::AlignLeading|Qt::AlignLeft|Qt::AlignTop</set>
src/qt/forms/hel
...

💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1618849607)
Updates:
- Rebased
- Implemented refactoring [suggestion](https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1245038873) from @stickies-v, moving other http reject validations into the constructor and regrouping them under a `Validate()` function to make the entire validation more consistent.

💬 furszy commented on pull request "Remove confusing "Dust" label from coincontrol / sendcoins dialog":
(https://github.com/bitcoin-core/gui/pull/719#discussion_r1251098387)
> It seems at least that we already have a couple of instances in master where both Qt::AlignLeading and Qt::AlignLeft are used together.

Quite sure that we have them because of QT creator setting the value automatically. Probably because there is some ancient qt version that doesn't have them as synonyms.

But for us, it should be fine to drop them, even qt 4.8 has them as synonyms ([link](https://het.as.utexas.edu/HET/Software/html/qt.html#AlignmentFlag-enum)).

💬 theStack commented on pull request "Remove confusing "Dust" label from coincontrol / sendcoins dialog":
(https://github.com/bitcoin-core/gui/pull/719#discussion_r1251103633)
@furszy: Thanks a lot for checking. I removed the redundant `Qt::AlignLeading` properties from the second commit, i.e. it's only `Qt::AlignLeft` remaining.

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1618871642)
Without going full cluster mempool, I think whatever linearization is given out, we should probably ensure that each remaining prefix of the ancestor package is an ancestor package itself, before attempting submission to the mempool. Skip the entry if it's not. Maybe with ancestor set scoring linearizer this is redundant? I can't tell. It's probably going to get a lot closer to the ordering you want regardless, as it catches things like https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1
...

💬 john-moffett commented on pull request "Exit and show error if unrecognized command line args are present":
(https://github.com/bitcoin-core/gui/pull/742#issuecomment-1618901090)
Modified to follow @hebasto 's idea of only allowing `bitcoin:` URIs if no other options follow.