💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1646397855)
@laanwj It's true that you generally shouldn't use `using namespace std;` in headers, but the C++ Core Guidelines actually have an [exception](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf7-dont-write-using-namespace-at-global-scope-in-a-header-file) for the standard literals. The reason being that literals without underscore are reserved for the standard library, so there is no risk of namespace pollution.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1646397855)
@laanwj It's true that you generally shouldn't use `using namespace std;` in headers, but the C++ Core Guidelines actually have an [exception](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf7-dont-write-using-namespace-at-global-scope-in-a-header-file) for the standard literals. The reason being that literals without underscore are reserved for the standard library, so there is no risk of namespace pollution.
✅ 0xB10C closed a pull request: "rpc: introduce getversion RPC"
(https://github.com/bitcoin/bitcoin/pull/30112)
(https://github.com/bitcoin/bitcoin/pull/30112)
💬 0xB10C commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2178998633)
While this has a few Concept ACKs, I'm not sure the Approach (introducing a new RPC) is really the best route here. I'm closing this for now as I'll be focusing on other projects. Feel free to pick this up if you want.
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2178998633)
While this has a few Concept ACKs, I'm not sure the Approach (introducing a new RPC) is really the best route here. I'm closing this for now as I'll be focusing on other projects. Feel free to pick this up if you want.
💬 glozow commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1646027082)
Needs documentation, i.e. mentioning the nonce should be randomly generated
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1646027082)
Needs documentation, i.e. mentioning the nonce should be randomly generated
💬 Fi3 commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2179108671)
@Sjors initially the TP was without noise, we added it mainly to have third party TPs (not pool and not miner). About easy of use, if we don't want to implement an sv1 translator in core, the miner still have to deploy a translator proxy. Maybe is easier to just release a translator that is also a JDC (like what I did for demand) rather the implement the JDC in the TP and add custom message to do that.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2179108671)
@Sjors initially the TP was without noise, we added it mainly to have third party TPs (not pool and not miner). About easy of use, if we don't want to implement an sv1 translator in core, the miner still have to deploy a translator proxy. Maybe is easier to just release a translator that is also a JDC (like what I did for demand) rather the implement the JDC in the TP and add custom message to do that.
📝 furszy opened a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309)
Small change. Found it while finishing my review on #29523. This does not interfere with it.
Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.
(https://github.com/bitcoin/bitcoin/pull/30309)
Small change. Found it while finishing my review on #29523. This does not interfere with it.
Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for `fundrawtransaction`.
📝 m3dwards opened a pull request: "ci: add option for running tests without volume"
(https://github.com/bitcoin/bitcoin/pull/30310)
Fixes: https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1645950272
Cache wasn't being saved when run on GHA because the default behaviour of the CI script was to store cache items in a docker volume. This works on Cirrus CI as the volumes are shared but it does not work on Github Actions in which each run is ephemeral.
Kept the default behaviour the same so hopefully this continues to work as is for the Cirrus CI jobs.
(https://github.com/bitcoin/bitcoin/pull/30310)
Fixes: https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1645950272
Cache wasn't being saved when run on GHA because the default behaviour of the CI script was to store cache items in a docker volume. This works on Cirrus CI as the volumes are shared but it does not work on Github Actions in which each run is ephemeral.
Kept the default behaviour the same so hopefully this continues to work as is for the Cirrus CI jobs.
💬 m3dwards commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1646598411)
https://github.com/bitcoin/bitcoin/pull/30310
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1646598411)
https://github.com/bitcoin/bitcoin/pull/30310
👍 brunoerg approved a pull request: "fuzz: Fix wallet_bdb_parser 32-bit unhandled fseek error"
(https://github.com/bitcoin/bitcoin/pull/30307#pullrequestreview-2128908068)
utACK fa7bc9bbca9348cf31b97bee0789ea7caeec635c
(https://github.com/bitcoin/bitcoin/pull/30307#pullrequestreview-2128908068)
utACK fa7bc9bbca9348cf31b97bee0789ea7caeec635c
💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1646607479)
Ah, yeah, I gave it a shot and it pretty simple so I am changing the return type as well in the refactoring commit now.
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1646607479)
Ah, yeah, I gave it a shot and it pretty simple so I am changing the return type as well in the refactoring commit now.
💬 am-sq commented on pull request "doc: loadwallet loads from relative walletdir":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646609836)
Based on https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#sqlite-database-based-wallets, it appears `.dat` files can be passed to sqlite wallets as well.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646609836)
Based on https://github.com/bitcoin/bitcoin/blob/master/doc/files.md#sqlite-database-based-wallets, it appears `.dat` files can be passed to sqlite wallets as well.
👋 am-sq's pull request is ready for review: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302)
(https://github.com/bitcoin/bitcoin/pull/30302)
💬 am-sq commented on issue "Improve description of the `filename` parameter of `loadwallet` RPC":
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2179363155)
Wanted to explicitly mention I have opened https://github.com/bitcoin/bitcoin/pull/30302 to resolve this issue. Thanks in advance for guidance and review.
(https://github.com/bitcoin/bitcoin/issues/30269#issuecomment-2179363155)
Wanted to explicitly mention I have opened https://github.com/bitcoin/bitcoin/pull/30302 to resolve this issue. Thanks in advance for guidance and review.
💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2179369598)
Needed to rebase because of a silent merge conflict.
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2179369598)
Needed to rebase because of a silent merge conflict.
🤔 hodlinator requested changes to a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2128956201)
nACK dc4cbe209508b2f95a748137b200056451405c9e
Commits need squashing together.
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2128956201)
nACK dc4cbe209508b2f95a748137b200056451405c9e
Commits need squashing together.
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646635390)
Tried `createwallet` to observe that the current version creates a directory with the given name, containing `wallet.dat` (and `wallet.dat-journal` while loaded).
Suggestion: put the normal/current/simplest operation first, and also add some descriptions to command examples:
```suggestion
"\nLoad regular wallet with files under wallets/foo/:\n"
+ HelpExampleCli("loadwallet", "\"foo\"") +
"\nLoad wallet using absolute path:\n"
...
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646635390)
Tried `createwallet` to observe that the current version creates a directory with the given name, containing `wallet.dat` (and `wallet.dat-journal` while loaded).
Suggestion: put the normal/current/simplest operation first, and also add some descriptions to command examples:
```suggestion
"\nLoad regular wallet with files under wallets/foo/:\n"
+ HelpExampleCli("loadwallet", "\"foo\"") +
"\nLoad wallet using absolute path:\n"
...
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646638086)
`/Users/joe/specialWallets` should be changed to `/Users/joe/specialWallet/` here and in the RPC example since we only load one wallet at a time and to make clear that it is a directory.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646638086)
`/Users/joe/specialWallets` should be changed to `/Users/joe/specialWallet/` here and in the RPC example since we only load one wallet at a time and to make clear that it is a directory.
💬 Prabhat1308 commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1646641952)
I got some data for about 4100+ transactions from a source (cant be sure of the credibility) for bitcoin transactions in a mempool and i ran a script to count the avg. parents which came to approx. 1.6 . Another way I thought of this is I got the avg. number of [inputs per transaction](https://transactionfee.info/charts/inputs-per-transaction/) . For 2023-2024 the average is somewhat between 2-3 . If we make a tree like dependency structure for transaction considering the transactions which had
...
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1646641952)
I got some data for about 4100+ transactions from a source (cant be sure of the credibility) for bitcoin transactions in a mempool and i ran a script to count the avg. parents which came to approx. 1.6 . Another way I thought of this is I got the avg. number of [inputs per transaction](https://transactionfee.info/charts/inputs-per-transaction/) . For 2023-2024 the average is somewhat between 2-3 . If we make a tree like dependency structure for transaction considering the transactions which had
...
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646668328)
Thanks for the suggestion - fixed.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646668328)
Thanks for the suggestion - fixed.
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646668494)
Great suggestion- added!
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1646668494)
Great suggestion- added!