💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1620535396)
After a chat with @stickies-v where we were discussing different approaches on this enhancement and other details regarding fuzz testing, libevent functionality and each commit intention, I've decided to put this onto draft. Firstly we would need to define the purpose of the `httprequest` object, do we want/ need the `httprequest` obj to exist even with an invalid request?, as @stickies-v [raised his concerns before](https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1481163868), it seem
...

💬 ajtowns commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620538136)
> I'm starting to think that something closer to your idea here is right: trying ancestor sets of every transaction in the linearization in order, if the ancestor set feerate is suffiicently high. This indeed won't deal with multiple-children-pay-for-parent cases perfectly, but including everything connected may be too much as well. I'll try to think about this more.

I don't think handling multiple-children-pay-for-parent cases perfectly should be a goal here -- we're not fixing eviction (or
...

💬 luke-jr commented on pull request "exclude ipc scheme from port check":
(https://github.com/bitcoin/bitcoin/pull/28020#issuecomment-1620538924)
nit: Rebasing onto bbbf89a9de0757c44880495244f90967f7147c0d would enable a clean merge to 25.x also

💬 MarcoFalke commented on pull request "descriptors: Add a KEY expression representing a list of individual keys":
(https://github.com/bitcoin/bitcoin/pull/26626#issuecomment-1620571802)
Needs rebase if still relevant

💬 MarcoFalke commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1620572868)
From CI https://cirrus-ci.com/task/4675435070488576?logs=ci#L3365:

```
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 337, in test_double_change
assert_equal(wallet.gettransaction(desc_tx['vin'][0]['txid'])['amount'], Decimal(50)) # assert input value
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i
...

💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1620574057)
> Looks like the fuzz target doesn't compile on windows?

Looks like this still wasn't addressed?

📝 luke-jr opened a pull request: "Fix issues in ZMQ error handling"
(https://github.com/bitcoin/bitcoin/pull/28029)
Behaves better if abnormal issues occur

💬 Sjors commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620591164)
If anyone on master is affected by this, we could perhaps add a command to wallet-tool that resets the descriptor ID and cache.

👍 brunoerg approved a pull request: "fuzz: Generate rpc fuzz targets individually"
(https://github.com/bitcoin/bitcoin/pull/28015#pullrequestreview-1513276185)
ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a

Running only with `rpc`, `targets` in `generate_corpus` becomes:
```
[('rpc', {'LIMIT_TO_RPC_COMMAND': 'analyzepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'clearbanned'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinepsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'combinerawtransaction'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'converttopsbt'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createmultisig'}), ('rpc', {'LIMIT_TO_RPC_COMMAND': 'createpsbt'}), ('rpc', {'LIMIT_
...

💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252291878)
> Are you proposing to limit the annex size including any tags to 257 bytes? I see the conceptual difference, but there wouldn't be a functional difference for now?

Yes, for now it doesn’t make a functional difference. The issue I have in mind is the following, assuming the unstructured annex format is deployed and we see applications building on top of it, the 256 (or 257 now) annex size limit will have to become a max-size data payload, which is going to leak in the all application toolchai
...

💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252299556)
> Maybe you don't want to use it for multi-party protocols at all because of that.

I think that’s a good question, there is an annex inflation risk concerning the multi-party protocol users and there is a new CPU DoS risk that is faced by node operators due to the increased in annex data size. I think this CPU DoS risk is coming from the fact that with BIP341, there is a `sha_annex` in the transaction digest if the annex is present and therefore you have a new SHA-256 ops and data bytes to ha
...

💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252300001)
See the CPU DoS concerned mentioned above about BIP341’s `sha_annex`, if annexes start to be relayed.

💬 luke-jr commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#issuecomment-1620640272)
The current PR doesn't make sense to me. It's treating a height as a time? But no block would have a time before a height... So this just adds calculations that do nothing?

💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#discussion_r1252303541)
> Especially if there are no concrete plans for multi-party protocols that require the unique properties of the annex.

If you’re taking the use-case of unstructured data discussed on the mailing list, I think there is still a leak in term of fee-bumping reserves than a user must provision for, in case of the worst annex inflation attacks being done, under known limits of `PER_INPUT_MAX_ANNEX_SIZE` (and eventual `MAX_ANNEX_BUDGET`).

Of course, as you’re raising you can always have an advers
...

💬 ariard commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1620650118)
Left a new round of reviews, I think the following conceptual concerns are pending:
- padding of the signaling byte to preserve evolvability for future consensus validation of the annex data
- adding a `MAX_ANNEX_BUDGET` or not to mitigate potential CPU DoS concerns for full-nodes, and potential impact for fee-bumping reserves
- opted-in of `annexrelay` for node operators
- annex policy limits versioned on the transaction’s `nVersion` field

I still have to reply on the latest mail post ab
...

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1252319116)
ACK, please resolve.

💬 dimitaracev commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1620693065)
ACK `5cf4427`

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1252331248)
> I must either be missing something about this line or it might be unnecessary. If I understand correctly this is updating `m_undo_height_in_last_blockfile` when the next block is connected after the flush happened. However, why not update it right away when the flush happens? I tried that by removing this line it seems like all tests are still passing without it.

It's not surprising that there's no test coverage for this. But to explain: the goal of the code is to flush the undo file when t
...

💬 JayBitron commented on pull request "exclude ipc scheme from port check":
(https://github.com/bitcoin/bitcoin/pull/28020#discussion_r1252364554)
`compare` throws an exception if the input string is lower than 4 character, but since this a conflict PR, please close it.

💬 ismaelsadeeq commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1252358645)
```c++
if (parent_feerate < individual_feerate) _removeTx(parent.GetTx().GetHash(), /* inBlock = */true);
```
This means parents that are confirmed from previous blocks are going to be removed as well right? not just parents from this block we are processing?