💬 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
...
💬 luke-jr commented on pull request "Use hardened runtime on macOS release builds.":
(https://github.com/bitcoin/bitcoin/pull/29127#issuecomment-1869813403)
>The release maintainer, or any authorized Apple Developer, will need to run xcrun notarytool to prevent gatekeeper warnings on macOS.
Isn't that just to fully close #15774? In other words, this PR seems like a strict step forward even if we don't go the rest of the way?
> Using xcrun staple to generate a binary that doesn't call home on first launch would be bonus, but at least this would massively improve the user experience.
I was under the impression that *not* doing the notary thin
...
(https://github.com/bitcoin/bitcoin/pull/29127#issuecomment-1869813403)
>The release maintainer, or any authorized Apple Developer, will need to run xcrun notarytool to prevent gatekeeper warnings on macOS.
Isn't that just to fully close #15774? In other words, this PR seems like a strict step forward even if we don't go the rest of the way?
> Using xcrun staple to generate a binary that doesn't call home on first launch would be bonus, but at least this would massively improve the user experience.
I was under the impression that *not* doing the notary thin
...
💬 luke-jr commented on pull request "wallet, rpc: add BIP44 `account` in `createwallet`":
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1869813973)
Feels like this could be confused with real/beancounter accounts like we used to support years ago. Maybe `hd_account` or something?
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1869813973)
Feels like this could be confused with real/beancounter accounts like we used to support years ago. Maybe `hd_account` or something?
💬 furszy commented on pull request "wallet: add meaningful error message and fix test":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869817009)
> > 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
...
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869817009)
> > 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
...
💬 dimitaracev commented on pull request "wallet: add meaningful error message and fix test":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869823524)
@furszy Oh I see, will continue to look into this further. Thanks.
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869823524)
@furszy Oh I see, will continue to look into this further. Thanks.
💬 ybaidiuk commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1869829376)
> Величезна кількість спаму, спричиненого написами в блокчейні біткойн, насправді викликає тривогу, його слід вирішувати комплексно та таким чином, щоб не завдати шкоди ринку BRC-20 або засобам існування людей.
>
> Деякі особи не враховують природу вразливості, вона не має нічого спільного з позбавленням від написів. Це пов’язано з перевантаженістю мережі, спричиненою такими типами транзакцій. Справа не лише у вищі комісії чи повільніший час обробки; ця ситуація створює серйозний ризик для ці
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1869829376)
> Величезна кількість спаму, спричиненого написами в блокчейні біткойн, насправді викликає тривогу, його слід вирішувати комплексно та таким чином, щоб не завдати шкоди ринку BRC-20 або засобам існування людей.
>
> Деякі особи не враховують природу вразливості, вона не має нічого спільного з позбавленням від написів. Це пов’язано з перевантаженістю мережі, спричиненою такими типами транзакцій. Справа не лише у вищі комісії чи повільніший час обробки; ця ситуація створює серйозний ризик для ці
...