π fanquake merged a pull request: "ci: Remove deprecated container.greedy"
(https://github.com/bitcoin/bitcoin/pull/28024)
(https://github.com/bitcoin/bitcoin/pull/28024)
π¬ luke-jr commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620528011)
Tag for backport?
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620528011)
Tag for backport?
π¬ fanquake commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620530627)
Not if this has (as I understand it) only been broken on master?
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620530627)
Not if this has (as I understand it) only been broken on master?
π¬ achow101 commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#discussion_r1252228384)
The descriptor isn't unknown. It's a corruption error as the id written in the database doesn't match the id that we calculate.
(https://github.com/bitcoin/bitcoin/pull/27920#discussion_r1252228384)
The descriptor isn't unknown. It's a corruption error as the id written in the database doesn't match the id that we calculate.
π¬ achow101 commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620534607)
> Tag for backport?
No need, only affects master.
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620534607)
> Tag for backport?
No need, only affects master.
π pablomartin4btc converted_to_draft a pull request: "httpserver, rest: improving URI validation"
(https://github.com/bitcoin/bitcoin/pull/27253)
This PR contains an isolated enhancement of the segfault bugfix https://github.com/bitcoin/bitcoin/pull/27468 (already merged), improving the way we handle the URI validation, which will be performed only once instead of on each query parameter of a rest service endpoint.
(https://github.com/bitcoin/bitcoin/pull/27253)
This PR contains an isolated enhancement of the segfault bugfix https://github.com/bitcoin/bitcoin/pull/27468 (already merged), improving the way we handle the URI validation, which will be performed only once instead of on each query parameter of a rest service endpoint.
π¬ 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
...
(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
...
(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
(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
(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
...
(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?
(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
(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.
(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_
...
(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
...
(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
...
(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.
(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?
(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
...
(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
...