💬 maflcko commented on pull request "doc: upgrade Bitcoin Core license to 2024":
(https://github.com/bitcoin/bitcoin/pull/29142#issuecomment-1868992681)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/29142#issuecomment-1868992681)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 dimitaracev commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1868993395)
Maybe we should return a proper message that indicates that the wallet we are trying to unload is already marked for unloading and we can catch that in the python code.
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-1868993395)
Maybe we should return a proper message that indicates that the wallet we are trying to unload is already marked for unloading and we can catch that in the python code.
💬 sipa commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1869001285)
@maflcko Constant propagation + strength reduction should make that code not emit an actual modulo operation in any compiler from the last 20 years.
But hopefully that pattern in libc++ also matches whatever pattern matching clang has to make sure it instead emits an actual rot instruction on platforms where it exists.
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1869001285)
@maflcko Constant propagation + strength reduction should make that code not emit an actual modulo operation in any compiler from the last 20 years.
But hopefully that pattern in libc++ also matches whatever pattern matching clang has to make sure it instead emits an actual rot instruction on platforms where it exists.
💬 Lajandra commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1869042800)
> https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515 :
>
> ```shell
> _20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
> node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
> node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/
...
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1869042800)
> https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515 :
>
> ```shell
> _20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
> node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
> node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/
...
💬 pinheadmz commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1869080735)
Ok I don't agree that that would be an improvement, because:
1. The path already contains the word "example" (`examples/bitcoin.conf`)
2. Changing that file's name would also require an additional comment in the header instructing the user to rename the file as well as copy it to their data directory
3. The default datadir for all platforms is very clear in several places in the docs, and that default datadir is never *inside* the code repository
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1869080735)
Ok I don't agree that that would be an improvement, because:
1. The path already contains the word "example" (`examples/bitcoin.conf`)
2. Changing that file's name would also require an additional comment in the header instructing the user to rename the file as well as copy it to their data directory
3. The default datadir for all platforms is very clear in several places in the docs, and that default datadir is never *inside* the code repository
💬 kashifs commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1869114446)
Concept ACK
I compiled from source, read through each modified line of code, ran the automated tests, and modified `test_maxfeerate_maxburn_submitpackage` test to check `maxfeerate` and `maxburnamount` seperately
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1869114446)
Concept ACK
I compiled from source, read through each modified line of code, ran the automated tests, and modified `test_maxfeerate_maxburn_submitpackage` test to check `maxfeerate` and `maxburnamount` seperately
💬 BitcoinMechanic commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1869194444)
Strong ACK.
1. It makes bloating the UTXO set trivial and this is being done with the recent "stamps" spam attack.
2. It's a poor way to do multisig (and arguments for stateless multisig aren't valid - it's not the responsibility of users running nodes to store data only relevant to you.)
It makes sense for nodes to refuse to relay unnecessarily burdensome TXs.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1869194444)
Strong ACK.
1. It makes bloating the UTXO set trivial and this is being done with the recent "stamps" spam attack.
2. It's a poor way to do multisig (and arguments for stateless multisig aren't valid - it's not the responsibility of users running nodes to store data only relevant to you.)
It makes sense for nodes to refuse to relay unnecessarily burdensome TXs.
💬 BitcoinMechanic commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1869197720)
Filtering is a very useful step. It is irrelevant to continually bring up the point that miners can still include this stuff in blocks.
Miners get to decide what goes in blocks and if the wider network is strongly indicating that it (correctly) considers a use-case to be undesirable they can choose to mine templates that do not include such "transactions". Until now there has been no signalling of this and no good option beyond solo mining. Ocean pool (disclaimer: my employer) provides this o
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1869197720)
Filtering is a very useful step. It is irrelevant to continually bring up the point that miners can still include this stuff in blocks.
Miners get to decide what goes in blocks and if the wider network is strongly indicating that it (correctly) considers a use-case to be undesirable they can choose to mine templates that do not include such "transactions". Until now there has been no signalling of this and no good option beyond solo mining. Ocean pool (disclaimer: my employer) provides this o
...
💬 da2ce7 commented on pull request "Ephemeral Anchors":
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-1869288197)
@luke-jr
> > 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.
This Proposed BIP brings a set of concepts that allows the community to communicate using a coherent language regarding what "Ephemeral Anchors" are. By defining this term in conte
...
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-1869288197)
@luke-jr
> > 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.
This Proposed BIP brings a set of concepts that allows the community to communicate using a coherent language regarding what "Ephemeral Anchors" are. By defining this term in conte
...
💬 GregTonoski commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1869411974)
> 1. The path already contains the word "example" (`examples/bitcoin.conf`)
It is not true. The path doesn't contain the word "example", see: https://bitcoincore.org/bin/bitcoin-core-26.0/bitcoin-26.0-win64.zip.
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1869411974)
> 1. The path already contains the word "example" (`examples/bitcoin.conf`)
It is not true. The path doesn't contain the word "example", see: https://bitcoincore.org/bin/bitcoin-core-26.0/bitcoin-26.0-win64.zip.
💬 GregTonoski commented on issue "Concerns about the Impacts of Ordinals Transactions on the Bitcoin Network":
(https://github.com/bitcoin/bitcoin/issues/26995#issuecomment-1869468616)
The most serious issue reported in years and immediately neglected. Why not reopening this issue in order to search for a solution/fix?
(https://github.com/bitcoin/bitcoin/issues/26995#issuecomment-1869468616)
The most serious issue reported in years and immediately neglected. Why not reopening this issue in order to search for a solution/fix?
💬 pinheadmz commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1869566796)
Aha ok sorry I misunderstood, you were referring to the release packages not the source code repo. Still I think the instructions in the header are clear. However it does seem reasonable to include a README file in the release packages with at least some minimal explanation of the contents of the package, inclusing how to use the example conf file and also identifying what the executables are in`bin/`.
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1869566796)
Aha ok sorry I misunderstood, you were referring to the release packages not the source code repo. Still I think the instructions in the header are clear. However it does seem reasonable to include a README file in the release packages with at least some minimal explanation of the contents of the package, inclusing how to use the example conf file and also identifying what the executables are in`bin/`.
📝 dimitaracev opened a pull request: "wallet: add meaningful error message and fix test"
(https://github.com/bitcoin/bitcoin/pull/29143)
Fixes https://github.com/bitcoin/bitcoin/issues/29073
Calling `unloadwallet` first marks that wallet for unloading and then unloads it. If the unloading does not finish in time and `unloadwallet` is called again for the same wallet an assertion error is thrown `wallet/wallet.cpp:246: void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertionit.second` which is not descriptive. The test in issue https://github.com/bitcoin/bitcoin/issues/29073 is sometimes failing due to that assertion. Th
...
(https://github.com/bitcoin/bitcoin/pull/29143)
Fixes https://github.com/bitcoin/bitcoin/issues/29073
Calling `unloadwallet` first marks that wallet for unloading and then unloads it. If the unloading does not finish in time and `unloadwallet` is called again for the same wallet an assertion error is thrown `wallet/wallet.cpp:246: void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertionit.second` which is not descriptive. The test in issue https://github.com/bitcoin/bitcoin/issues/29073 is sometimes failing due to that assertion. Th
...
💬 dimitaracev commented on pull request "wallet: add meaningful error message and fix test":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869707247)
cc @maflcko @fanquake
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869707247)
cc @maflcko @fanquake
💬 brunoerg commented on pull request "wallet: add meaningful error message and fix test":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869709929)
```sh
test/functional/wallet_multiwallet.py:38:9: F841 local variable 'e' is assigned to but never used
test/functional/wallet_multiwallet.py:39:50: F821 undefined name 'e'
test/functional/wallet_multiwallet.py:39:104: F821 undefined name 'e'
test/functional/wallet_multiwallet.py:40:63: F821 undefined name 'e'
test/functional/wallet_multiwallet.py:40:130: F821 undefined name 'e'
```
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869709929)
```sh
test/functional/wallet_multiwallet.py:38:9: F841 local variable 'e' is assigned to but never used
test/functional/wallet_multiwallet.py:39:50: F821 undefined name 'e'
test/functional/wallet_multiwallet.py:39:104: F821 undefined name 'e'
test/functional/wallet_multiwallet.py:40:63: F821 undefined name 'e'
test/functional/wallet_multiwallet.py:40:130: F821 undefined name 'e'
```
✅ 22388o closed a pull request: "doc: upgrade Bitcoin Core license to 2024"
(https://github.com/bitcoin/bitcoin/pull/29142)
(https://github.com/bitcoin/bitcoin/pull/29142)
🤔 furszy reviewed a pull request: "wallet: add meaningful error message and fix test"
(https://github.com/bitcoin/bitcoin/pull/29143#pullrequestreview-1796616389)
How is this possible if `RemoveWallet()` locks the context wallets' mutex, checks if the wallet is there (failing if not) and then removes it.
(https://github.com/bitcoin/bitcoin/pull/29143#pullrequestreview-1796616389)
How is this possible if `RemoveWallet()` locks the context wallets' mutex, checks if the wallet is there (failing if not) and then removes it.
📝 furszy opened a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144)
Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144).
Instead of throwing the 'Unable to parse settings file' error when the file is just empty.
We can continue execution as if nothing happened.
(https://github.com/bitcoin/bitcoin/pull/29144)
Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144).
Instead of throwing the 'Unable to parse settings file' error when the file is just empty.
We can continue execution as if nothing happened.
💬 luke-jr commented on pull request "test: Add test case for speding bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1869811438)
Title has a typo. Maybe should test that it works with both values for `-permitbaremultisig`?
(https://github.com/bitcoin/bitcoin/pull/29120#issuecomment-1869811438)
Title has a typo. Maybe should test that it works with both values for `-permitbaremultisig`?
💬 dimitaracev commented on pull request "wallet: add meaningful error message and fix test":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869811694)
> How is this possible if `RemoveWallet()` locks the context wallets' mutex, checks if the wallet is there (failing if not) and then removes it.
From my understanding, the statements
```c++
auto it = g_unloading_wallet_set.insert(name);
assert(it.second);
```
assume that the wallet will always be correctly inserted into `g_unloading_wallet_set` where `g_unloading_wallet_set.insert(name)` returns a pair (`it.second` is 1 if the wallet is successfully inserted into the set to be used by `R
...
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869811694)
> How is this possible if `RemoveWallet()` locks the context wallets' mutex, checks if the wallet is there (failing if not) and then removes it.
From my understanding, the statements
```c++
auto it = g_unloading_wallet_set.insert(name);
assert(it.second);
```
assume that the wallet will always be correctly inserted into `g_unloading_wallet_set` where `g_unloading_wallet_set.insert(name)` returns a pair (`it.second` is 1 if the wallet is successfully inserted into the set to be used by `R
...