💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1831538454)
You are right. If ``data.nTime`` is bigger than ``end_of_chain_timestamp`` we could go into the case where ``fTxTotal`` is smaller than ``pindex->m_chain_tx_count`` .
Maybe @sipa can explain why thinks that after the changes this clamp could be removed? As mentioned in https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432418782.
For the moment, I will add it again. Thanks for the observation!
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1831538454)
You are right. If ``data.nTime`` is bigger than ``end_of_chain_timestamp`` we could go into the case where ``fTxTotal`` is smaller than ``pindex->m_chain_tx_count`` .
Maybe @sipa can explain why thinks that after the changes this clamp could be removed? As mentioned in https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2432418782.
For the moment, I will add it again. Thanks for the observation!
🤔 ismaelsadeeq reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2419167788)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2419167788)
Concept ACK
📝 maflcko opened a pull request: "fuzz: Limit wallet_notifications iterations"
(https://github.com/bitcoin/bitcoin/pull/31238)
I don't think the fuzz target has ever found a real issue. The closest being https://github.com/bitcoin/bitcoin/pull/25869
It is also, by far, the slowest fuzz target. For example, looking at https://cirrus-ci.com/task/5533338067271680?logs=ci#L3974, it takes more than one hour:
```
Run wallet_notifications with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_corpora/wallet_notifications
...
(https://github.com/bitcoin/bitcoin/pull/31238)
I don't think the fuzz target has ever found a real issue. The closest being https://github.com/bitcoin/bitcoin/pull/25869
It is also, by far, the slowest fuzz target. For example, looking at https://cirrus-ci.com/task/5533338067271680?logs=ci#L3974, it takes more than one hour:
```
Run wallet_notifications with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_corpora/wallet_notifications
...
💬 achow101 commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2460749631)
ACK d22a234ed270286b483aec2db1e2f716b9756231
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2460749631)
ACK d22a234ed270286b483aec2db1e2f716b9756231
💬 l0rinc commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2460759546)
@maciejsszmigiero, are you still working on this or should we take over?
---
I can also confirm that it's possible to just switch file size values back-and-forth without needing a reindex.
I have reindexed until block 600k with master vs 16 mb blocks (instead of the 128 for the reasons mentioned before).
The LevelDB files seem to effortlessly change from 2 mb to 17 :
* **chainstate/062435.ldb - 906'412 bytes**
* chainstate/061885.ldb - 2'171'330 bytes
* chainstate/063212.ldb - 1'936
...
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2460759546)
@maciejsszmigiero, are you still working on this or should we take over?
---
I can also confirm that it's possible to just switch file size values back-and-forth without needing a reindex.
I have reindexed until block 600k with master vs 16 mb blocks (instead of the 128 for the reasons mentioned before).
The LevelDB files seem to effortlessly change from 2 mb to 17 :
* **chainstate/062435.ldb - 906'412 bytes**
* chainstate/061885.ldb - 2'171'330 bytes
* chainstate/063212.ldb - 1'936
...
🤔 mzumsande reviewed a pull request: "rpc: Remove submitblock pre-checks"
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2419408412)
Concept ACK
I agree with the general motivation. While this PR can change the error message of `submitblock` in case more than one thing is wrong with the block, I don't really see why one is more important than the other.
> UpdateUncommittedBlockStructures -> GetWitnessCommitmentIndex which would result in UB, before any PoW checks are made.
Nice find!
I think that it would be good to have a test that would trigger the UB if https://github.com/bitcoin/bitcoin/commit/ada0caa165905b50db
...
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2419408412)
Concept ACK
I agree with the general motivation. While this PR can change the error message of `submitblock` in case more than one thing is wrong with the block, I don't really see why one is more important than the other.
> UpdateUncommittedBlockStructures -> GetWitnessCommitmentIndex which would result in UB, before any PoW checks are made.
Nice find!
I think that it would be good to have a test that would trigger the UB if https://github.com/bitcoin/bitcoin/commit/ada0caa165905b50db
...
🚀 achow101 merged a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164)
(https://github.com/bitcoin/bitcoin/pull/31164)
✅ achow101 closed a pull request: "tests: Handle BDB dynamic pagesize"
(https://github.com/bitcoin/bitcoin/pull/31222)
(https://github.com/bitcoin/bitcoin/pull/31222)
💬 achow101 commented on pull request "tests: Handle BDB dynamic pagesize":
(https://github.com/bitcoin/bitcoin/pull/31222#issuecomment-2460788291)
> Note that a similar change is also included in #30125,
Oh, right, forgot about that PR. Closing in favor of it.
(https://github.com/bitcoin/bitcoin/pull/31222#issuecomment-2460788291)
> Note that a similar change is also included in #30125,
Oh, right, forgot about that PR. Closing in favor of it.
💬 achow101 commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#issuecomment-2460794766)
ACK d45eb3964f693eddcf96f1e4083cf19d327be989
(https://github.com/bitcoin/bitcoin/pull/30125#issuecomment-2460794766)
ACK d45eb3964f693eddcf96f1e4083cf19d327be989
💬 brunoerg commented on pull request "fuzz: Limit wallet_notifications iterations":
(https://github.com/bitcoin/bitcoin/pull/31238#issuecomment-2460821784)
Besides being the slowest, I think this target has poor stability, around 40%.
cc: @marcofleon
(https://github.com/bitcoin/bitcoin/pull/31238#issuecomment-2460821784)
Besides being the slowest, I think this target has poor stability, around 40%.
cc: @marcofleon
💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2460858505)
> I've interpreted (and implemented) it as also allowing the taproot internal key, not just the output key. I think that is actually what I meant when writing the BIP, but it's been a while.
I think it's important that the (aggregate) plain key used in the key of `PSBT_IN_MUSIG2_PUB_NONCE`/`PSBT_IN_MUSIG2_PARTIAL_SIGNATURE` is unambiguous and clearly specified, or implementations will essentially have to try both for both in order to find in the PSBT all the pubnonces/musig_partial_signatures
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2460858505)
> I've interpreted (and implemented) it as also allowing the taproot internal key, not just the output key. I think that is actually what I meant when writing the BIP, but it's been a while.
I think it's important that the (aggregate) plain key used in the key of `PSBT_IN_MUSIG2_PUB_NONCE`/`PSBT_IN_MUSIG2_PARTIAL_SIGNATURE` is unambiguous and clearly specified, or implementations will essentially have to try both for both in order to find in the PSBT all the pubnonces/musig_partial_signatures
...
👍 brunoerg approved a pull request: "fuzz: Limit wallet_notifications iterations"
(https://github.com/bitcoin/bitcoin/pull/31238#pullrequestreview-2419524919)
code review ACK fa461d7a43aa5a321278c8f734e80fb7aa79bfdb
(https://github.com/bitcoin/bitcoin/pull/31238#pullrequestreview-2419524919)
code review ACK fa461d7a43aa5a321278c8f734e80fb7aa79bfdb
💬 laanwj commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1831772618)
> WDYT about creating a constant tuple for these values? They seem pretty generic enough.
are they needed anywhere else? in #31222 i suggested
```python
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0)
```
but honestly, enumerating all 8 possible values here seems fine
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1831772618)
> WDYT about creating a constant tuple for these values? They seem pretty generic enough.
are they needed anywhere else? in #31222 i suggested
```python
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0)
```
but honestly, enumerating all 8 possible values here seems fine
💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2460946238)
> Using the same public key that can be used to verify the final signature (therefore, the tweaked taproot pubkey for keypath spends, or the pubkey as it appears in tapleaves for script spends) seems the most natural choice to me.
That's a good point, I don't feel too strongly about this, it was just a bit more annoying to figure out how. I've updated the PR to do that.
Also opened https://github.com/bitcoin/bips/pull/1695 to clarify in the BIP.
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2460946238)
> Using the same public key that can be used to verify the final signature (therefore, the tweaked taproot pubkey for keypath spends, or the pubkey as it appears in tapleaves for script spends) seems the most natural choice to me.
That's a good point, I don't feel too strongly about this, it was just a bit more annoying to figure out how. I've updated the PR to do that.
Also opened https://github.com/bitcoin/bips/pull/1695 to clarify in the BIP.
💬 kanzure commented on pull request "doc: Update the developer mailing list address.":
(https://github.com/bitcoin/bitcoin/pull/29782#issuecomment-2460974096)
> For future reference, in case we decide to update the archive links cited in other places of the codebase, here is the mapping:
Unfortunately, lists.linuxfoundation.org is no longer hosting any mailing list archives, not just for bitcoin-dev but all the other mailing lists too.
Are there any particular preferences on doing that proposed replace-all update now that lists.linuxfoundation.org has pulled the content?
I see a few options here:
1) Wait and see if they put it back up.
...
(https://github.com/bitcoin/bitcoin/pull/29782#issuecomment-2460974096)
> For future reference, in case we decide to update the archive links cited in other places of the codebase, here is the mapping:
Unfortunately, lists.linuxfoundation.org is no longer hosting any mailing list archives, not just for bitcoin-dev but all the other mailing lists too.
Are there any particular preferences on doing that proposed replace-all update now that lists.linuxfoundation.org has pulled the content?
I see a few options here:
1) Wait and see if they put it back up.
...
💬 fjahr commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2460987257)
Concept ACK on removing libevent and replacing the HTTP server with our own code. FWIW, I have been working on the removal of libevent from torcontrol but I still see some errors that I need to debug. I hope I can open a PR soon.
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2460987257)
Concept ACK on removing libevent and replacing the HTTP server with our own code. FWIW, I have been working on the removal of libevent from torcontrol but I still see some errors that I need to debug. I hope I can open a PR soon.
🤔 hodlinator reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2414994647)
Code reviewed up until most of aa7c90a91adcf8ad9d6b94d5d17797cfd0eb5228.
Disclaimer: I posses far from full context when it comes to validation code, but at least as far as I can tell we are only modifying policy, not consensus.
PR title should probably have "p2p:" or some [other prefix](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request).
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2414994647)
Code reviewed up until most of aa7c90a91adcf8ad9d6b94d5d17797cfd0eb5228.
Disclaimer: I posses far from full context when it comes to validation code, but at least as far as I can tell we are only modifying policy, not consensus.
PR title should probably have "p2p:" or some [other prefix](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request).
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829045123)
nit: Missing word
```suggestion
# Dust is 1 satoshi since create_self_transfer_multi disallows 0
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829045123)
nit: Missing word
```suggestion
# Dust is 1 satoshi since create_self_transfer_multi disallows 0
```
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829048604)
nit: I like the transaction being called "dusty" as it creates dust but is not itself dust. Dust refers only to outputs, not to transactions. Would prefer `dusty_txid`.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829048604)
nit: I like the transaction being called "dusty" as it creates dust but is not itself dust. Dust refers only to outputs, not to transactions. Would prefer `dusty_txid`.