💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259013655)
`180000`, `210000`, e.t.c
To reduce the magic numbers we can create a variable for the versions, e.g `v18 = 180000`and reuse it throughout the tests?

💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1259064139)
So `m_recent_rejects_reconsiderable` logs transaction on its wtxid (which is good to avoid witness inflation changing the result) however isn’t that two restrictive if you have PKG1 submitted with A+B, A is too low with B and then you have A+B’ where A is good enough ? Or is that `TX_LOW_FEE` when the transaction isn’t good enough to meet “mempool min fee not meet” / “min relay fee not met” ?

If it’s correct, I would still favor to exempt package transaction from min relay fee checks as it’s
...

✅ fanquake closed a pull request: "Create generator-generic-ossf-slsa3-publish.yml"
(https://github.com/bitcoin-core/gui/pull/746)

📝 fanquake locked a pull request: "Create generator-generic-ossf-slsa3-publish.yml"
(https://github.com/bitcoin-core/gui/pull/746)

<!--
*** 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 or new tests that impr
...

💬 MarcoFalke commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1630123482)
I think you'll still need to rebase or fix the compile error

💬 MarcoFalke commented on issue "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin":
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1630141468)
See https://github.com/bitcoin/bitcoin/issues/17779 for further steps

✅ MarcoFalke closed an issue: "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin"
(https://github.com/bitcoin/bitcoin/issues/28049)

💬 dsldsl commented on issue "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin":
(https://github.com/bitcoin/bitcoin/issues/28049#issuecomment-1630168865)
that's helpful thank you

⚠️ meglio opened an issue: "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)"
(https://github.com/bitcoin/bitcoin/issues/28064)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)

when calling through rpc api, throws an error:

```
Internal bug detected: "isValid == error_msg.empty()"
rpc/output_script.cpp:64 (operator())
Bitcoin Core v25.0.0
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
```

### Expected behaviour

Address validation to return is_v
...

💬 MarcoFalke commented on issue "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)":
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630193718)
This will be fixed in 25.1, see 725c3dc2dd1402d631e05ef461fe2abbac9a008f

💬 S3RK commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259265158)
agree, you can resolve this

💬 S3RK commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1630248097)
Approach ACK

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1259300260)
In theory the waste score should be the same.

Because not all of the params are available within BnB we call
`result.ComputeAndSetWaste(cost_of_change, cost_of_change, 0);`
instead of
`result.ComputeAndSetWaste(min_viable_change, cost_of_change, change_fee);`

I believe this should be fine as BnB will not produce results with more than `cost_of_change` excess and therefore `GetChange()` will always return 0 and then `change_fee` doesn't matter.

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1259306993)
Yes, the discount value is a function of the effective fee rate, therefore it has to be taken into account in waste calculation. The problem is that `GetSelectionWaste` doesn't use `SelectionResult.GetEffectiveValue()` which already accounts for bump fee discount.

💬 fanquake commented on issue "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)":
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630335753)
Closing, given the fix has been backported. @meglio if you're able to compile the 25.x branch, and confirm this fixes your issue, please do.

✅ fanquake closed an issue: "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)"
(https://github.com/bitcoin/bitcoin/issues/28064)

💬 russeree commented on issue "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)":
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630337550)
This is currently resolved in master via the addition of an else case to ```if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec.data.begin() + 1, dec.data.end()))```

here

https://github.com/bitcoin/bitcoin/blob/ef29d5d7e239b42269dd22ea94a709b5e4ceb5e5/src/key_io.cpp#L194-L197

💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259398907)
1. This should not have a period '.' at the end. Since it is inconsistent with the rest of the documentation.
2. Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This)
3. The language used in BIP22 is a more concise definition ```"longpollid" of job to monitor for expiration; required and valid only for long poll requests```

🤔 russeree requested changes to a pull request: "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998"
(https://github.com/bitcoin/bitcoin/pull/28056#pullrequestreview-1523750256)
Some of my thoughts about this PR. Mostly regarding the grammatical consistency of these messages.

💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259399690)
Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This). Similar to review item number one.