💬 ajtowns commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1858768140)
> Right, seems fine to keep the assert. I was just thinking that it would be nice to drop it if the compiler can prove it isn't needed. Currently that only works for std::array or `[]` types. However, it doesn't seem to work out of the box for array wrappers, like uint256, whose extent is known at compile time.
So, number one, I think you need an helper to implicitly convert a uint256 to a constant-extent span or you'll have to be explicit everywhere:
```c++
class base_blob
{
...
c
...
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1858768140)
> Right, seems fine to keep the assert. I was just thinking that it would be nice to drop it if the compiler can prove it isn't needed. Currently that only works for std::array or `[]` types. However, it doesn't seem to work out of the box for array wrappers, like uint256, whose extent is known at compile time.
So, number one, I think you need an helper to implicitly convert a uint256 to a constant-extent span or you'll have to be explicit everywhere:
```c++
class base_blob
{
...
c
...
✅ ajtowns closed a pull request: "NOMERGE UFG Default permitbaremultisig=0"
(https://github.com/bitcoin/bitcoin/pull/29093)
(https://github.com/bitcoin/bitcoin/pull/29093)
💬 ajtowns commented on pull request "NOMERGE UFG Default permitbaremultisig=0":
(https://github.com/bitcoin/bitcoin/pull/29093#issuecomment-1858768492)
Okay closing this as up for grabs; should be easy if anyone wants to pick it up.
(https://github.com/bitcoin/bitcoin/pull/29093#issuecomment-1858768492)
Okay closing this as up for grabs; should be easy if anyone wants to pick it up.
💬 0xB10C commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1428780378)
> > The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in the unit test). This would leave us with quite a few additional addresses already in the tried table.
>
> these collisions would only affect future tests added after getaddrmaninfo/getrawaddrman tests though.
We'd need to clean up after we found a collision.
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1428780378)
> > The easiest way of producing a collision is by adding more and more addresses to the tried table (as we do in the unit test). This would leave us with quite a few additional addresses already in the tried table.
>
> these collisions would only affect future tests added after getaddrmaninfo/getrawaddrman tests though.
We'd need to clean up after we found a collision.
💬 maflcko commented on issue "Flaky `wallet_transactiontime_rescan.py --legacy-wallet` functional test":
(https://github.com/bitcoin/bitcoin/issues/28221#issuecomment-1858812580)
Could bump the timeout inside this test to fix it?
(https://github.com/bitcoin/bitcoin/issues/28221#issuecomment-1858812580)
Could bump the timeout inside this test to fix it?
💬 TheCharlatan commented on pull request "doc: Rework guix docs after 1.4 release":
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1428813691)
Not really related to this patch, but why is this suggesting to turn networking on and off? Seems like an efficient way to lock yourself out of a box.
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1428813691)
Not really related to this patch, but why is this suggesting to turn networking on and off? Seems like an efficient way to lock yourself out of a box.
💬 TheCharlatan commented on pull request "doc: Rework guix docs after 1.4 release":
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1428810054)
`s/packages/packaged/`
(https://github.com/bitcoin/bitcoin/pull/28962#discussion_r1428810054)
`s/packages/packaged/`
📝 Retropex reopened a pull request: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217)
The default activation of the `permitbaremultisig=0` option proposes an enhancement for the Bitcoin network. By refusing non-P2SH multisignature transactions from the outset, this modification would contribute to reducing spam attempts and maintaining a healthy decentralization by discouraging undesirable activities.
(https://github.com/bitcoin/bitcoin/pull/28217)
The default activation of the `permitbaremultisig=0` option proposes an enhancement for the Bitcoin network. By refusing non-P2SH multisignature transactions from the outset, this modification would contribute to reducing spam attempts and maintaining a healthy decentralization by discouraging undesirable activities.
💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428842615)
> Whatever argument is given to -datadir=, always make (if necessary) and use a subdirectory (within the specified directory) called test_bitcoin. That way, even if the user specifies their real data directory, all the test stuff will be within this subdirectory. (It's similar to how there already are subdirectories for testnet, signet, and regtest.)
Sounds good. s/test_bitcoin/tests (we don't use "signet_bitcoin", "testnet_bitcoin", "regtest_bitcoin").
------------
Also, probably somet
...
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428842615)
> Whatever argument is given to -datadir=, always make (if necessary) and use a subdirectory (within the specified directory) called test_bitcoin. That way, even if the user specifies their real data directory, all the test stuff will be within this subdirectory. (It's similar to how there already are subdirectories for testnet, signet, and regtest.)
Sounds good. s/test_bitcoin/tests (we don't use "signet_bitcoin", "testnet_bitcoin", "regtest_bitcoin").
------------
Also, probably somet
...
💬 fanquake commented on pull request "NOMERGE UFG Default permitbaremultisig=0":
(https://github.com/bitcoin/bitcoin/pull/29093#issuecomment-1858860418)
Removed up for grabs given #28217 has been reopened.
(https://github.com/bitcoin/bitcoin/pull/29093#issuecomment-1858860418)
Removed up for grabs given #28217 has been reopened.
👋 Retropex's pull request is ready for review: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217)
(https://github.com/bitcoin/bitcoin/pull/28217)
💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1858875806)
A substantial wave of P2MS txs containing false public keys are currently used for adding arbitrary data to the chain. They add significant amounts of data to the UTXOs set and thus make it expensive to execute a node without the possibility of pruning them.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1858875806)
A substantial wave of P2MS txs containing false public keys are currently used for adding arbitrary data to the chain. They add significant amounts of data to the UTXOs set and thus make it expensive to execute a node without the possibility of pruning them.
👍 luke-jr approved a pull request: "Update doc/policy/README.md"
(https://github.com/bitcoin/bitcoin/pull/29095#pullrequestreview-1785255931)
utACK, but maybe we should just have a table with them all somewhere instead?
(https://github.com/bitcoin/bitcoin/pull/29095#pullrequestreview-1785255931)
utACK, but maybe we should just have a table with them all somewhere instead?
💬 luke-jr commented on pull request "Ephemeral Anchors":
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-1858907861)
>BIP text here: https://github.com/instagibbs/bips/blob/ephemeral_anchor/bip-ephemeralanchors.mediawiki
Node policy is not a subject matter for standardization in itself. It's the transaction format that the BIP (if there is one) should focus on, not speculative behaviour of individual nodes.
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-1858907861)
>BIP text here: https://github.com/instagibbs/bips/blob/ephemeral_anchor/bip-ephemeralanchors.mediawiki
Node policy is not a subject matter for standardization in itself. It's the transaction format that the BIP (if there is one) should focus on, not speculative behaviour of individual nodes.
💬 AdamISZ commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1858929206)
@ryanofsky I just wanted to give feedback that this kind of bit me [\*], in running Joinmarket tests - the `pytest` invocation just started hanging when using Bitcoin Core 26.0 and it took some investigating to see why. Not disastrous of course, but I was left bewildered - why would Core be complaining that there existed another `bitcoin.conf` in `~/.bitcoin` when I had explicitly set `-conf=/somewhere/else/bitcoin.conf`? I've skimmed the thread but I don't really get the logic. Is it something
...
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1858929206)
@ryanofsky I just wanted to give feedback that this kind of bit me [\*], in running Joinmarket tests - the `pytest` invocation just started hanging when using Bitcoin Core 26.0 and it took some investigating to see why. Not disastrous of course, but I was left bewildered - why would Core be complaining that there existed another `bitcoin.conf` in `~/.bitcoin` when I had explicitly set `-conf=/somewhere/else/bitcoin.conf`? I've skimmed the thread but I don't really get the logic. Is it something
...
🤔 pablomartin4btc reviewed a pull request: "Wallet: Calculate used balance from GetBalance(...)"
(https://github.com/bitcoin/bitcoin/pull/29062#pullrequestreview-1785308728)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29062#pullrequestreview-1785308728)
Concept ACK
💬 pablomartin4btc commented on pull request "Wallet: Calculate used balance from GetBalance(...)":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1428925448)
What about caculating `avoid_reuse` locally (e.g. `const bool avoid = !(m_wallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE));` at the beginning of the function), instead of passing it? I thought @achow101's [comment](https://github.com/bitcoin/bitcoin/pull/28776/files#r1407875236) was referring to include that too but maybe I'm wrong.
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1428925448)
What about caculating `avoid_reuse` locally (e.g. `const bool avoid = !(m_wallet->IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE));` at the beginning of the function), instead of passing it? I thought @achow101's [comment](https://github.com/bitcoin/bitcoin/pull/28776/files#r1407875236) was referring to include that too but maybe I'm wrong.
💬 andrewtoth commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#discussion_r1428946493)
The purpose of `ReallocateCache` originally was to balance the snapshot and background chainstate caches (see https://github.com/bitcoin/bitcoin/pull/18637). However, in https://github.com/bitcoin/bitcoin/pull/25325/commits/5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 this function was removed from `ResizeCoinsCaches` and moved to `Flush`. This created an implicit assumption that calling `Flush` would clear the memory in the cache. This change violates that assumption, since the memory will no longe
...
(https://github.com/bitcoin/bitcoin/pull/28945#discussion_r1428946493)
The purpose of `ReallocateCache` originally was to balance the snapshot and background chainstate caches (see https://github.com/bitcoin/bitcoin/pull/18637). However, in https://github.com/bitcoin/bitcoin/pull/25325/commits/5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 this function was removed from `ResizeCoinsCaches` and moved to `Flush`. This created an implicit assumption that calling `Flush` would clear the memory in the cache. This change violates that assumption, since the memory will no longe
...
💬 andrewtoth commented on pull request "sync: improve CCoinsViewCache ReallocateCache":
(https://github.com/bitcoin/bitcoin/pull/28945#discussion_r1428961040)
Should this parameter be added to the fuzz targets or unit tests?
(https://github.com/bitcoin/bitcoin/pull/28945#discussion_r1428961040)
Should this parameter be added to the fuzz targets or unit tests?
🤔 luke-jr reviewed a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-1785323162)
Concept NACK, refactoring without a purpose, and probably makes more work for merging BIP8.
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-1785323162)
Concept NACK, refactoring without a purpose, and probably makes more work for merging BIP8.