💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1458819504)
Thank you for the review! These are valuable and I hope they allow me to improve future commits. I'm pushing changes addressing comments up to the 4th commit now.
Updated a1db3966d1c4f4213d7bfd6be0ea7fb308fe4e11 -> 65d026b47270c2fb9d2dab7f3d448b4abc37b93a ([tc/2022-09-libbitcoinkernel-chainparams-args_0](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_0) -> [tc/2022-09-libbitcoinkernel-chainparams-args_1](https://github.com/TheCharlatan/bitcoin/co
...
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1458819504)
Thank you for the review! These are valuable and I hope they allow me to improve future commits. I'm pushing changes addressing comments up to the 4th commit now.
Updated a1db3966d1c4f4213d7bfd6be0ea7fb308fe4e11 -> 65d026b47270c2fb9d2dab7f3d448b4abc37b93a ([tc/2022-09-libbitcoinkernel-chainparams-args_0](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_0) -> [tc/2022-09-libbitcoinkernel-chainparams-args_1](https://github.com/TheCharlatan/bitcoin/co
...
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128545452)
Thank you for spelling this out, I updated the description.
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128545452)
Thank you for spelling this out, I updated the description.
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128547248)
Good catch, updated. [diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_0..tc/2022-09-libbitcoinkernel-chainparams-args_1)
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128547248)
Good catch, updated. [diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_0..tc/2022-09-libbitcoinkernel-chainparams-args_1)
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128549764)
Thank you for taking the time to get into this. I hope the update I pushed is more in style. [diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_0..tc/2022-09-libbitcoinkernel-chainparams-args_1)
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128549764)
Thank you for taking the time to get into this. I hope the update I pushed is more in style. [diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_0..tc/2022-09-libbitcoinkernel-chainparams-args_1)
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128550399)
Good point, updated. [diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_0..tc/2022-09-libbitcoinkernel-chainparams-args_1)
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128550399)
Good point, updated. [diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_0..tc/2022-09-libbitcoinkernel-chainparams-args_1)
💬 jonatack commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1458861089)
Concept ACK, thanks for working on this.
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1458861089)
Concept ACK, thanks for working on this.
💬 jamesob commented on pull request "assumeutxo: background validation completion":
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1458876649)
> Missed this on last review (sorry):
Fixed!
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1458876649)
> Missed this on last review (sorry):
Fixed!
💬 willcl-ark commented on issue "Rescanning use only 1 core":
(https://github.com/bitcoin/bitcoin/issues/19992#issuecomment-1458921201)
@MarcoFalke this can be closed as `zapwallettxes` is removed?
(https://github.com/bitcoin/bitcoin/issues/19992#issuecomment-1458921201)
@MarcoFalke this can be closed as `zapwallettxes` is removed?
💬 junderw commented on issue "Allow several OP_RETURN in one tx and no limited size":
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1458921569)
> then could someone please do it? (you?)
There are many C++ devs you could pay to have this PR done for you. That would be much quicker, especially considering how simple the change is. The only hard part might be writing tests, since the testing harness etc. does take a bit of getting used to... (that said, looking at the other tests usually can give one a good head start)
There are also many bounty websites where you can lock BTC into a vault that is only released to the person who does
...
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1458921569)
> then could someone please do it? (you?)
There are many C++ devs you could pay to have this PR done for you. That would be much quicker, especially considering how simple the change is. The only hard part might be writing tests, since the testing harness etc. does take a bit of getting used to... (that said, looking at the other tests usually can give one a good head start)
There are also many bounty websites where you can lock BTC into a vault that is only released to the person who does
...
📝 ryanofsky opened a pull request: "refactor: Remove CAddressBookData::destdata"
(https://github.com/bitcoin/bitcoin/pull/27224)
This is cleanup that doesn't change external behavior. Benefits of the cleanup are:
- Removes awkward `StringMap` intermediate representation for wallet address metadata.
- Simplifies `CWallet`, deals with used address and received request serialization in `walletdb.cpp` instead of higher level wallet code
- Adds test coverage
This PR doesn't change externally observable behavior. Internally, the only change in behavior is that `EraseDestData` deletes rows directly from the database beca
...
(https://github.com/bitcoin/bitcoin/pull/27224)
This is cleanup that doesn't change external behavior. Benefits of the cleanup are:
- Removes awkward `StringMap` intermediate representation for wallet address metadata.
- Simplifies `CWallet`, deals with used address and received request serialization in `walletdb.cpp` instead of higher level wallet code
- Adds test coverage
This PR doesn't change externally observable behavior. Internally, the only change in behavior is that `EraseDestData` deletes rows directly from the database beca
...
💬 junderw commented on issue "Allow several OP_RETURN in one tx and no limited size":
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1458924629)
@MarcoFalke perhaps this could also use a "good first issue" tag? Seems pretty straight forward.
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1458924629)
@MarcoFalke perhaps this could also use a "good first issue" tag? Seems pretty straight forward.
👍 stickies-v approved a pull request: "doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2"
(https://github.com/bitcoin/bitcoin/pull/26968)
ACK 3e947d7117c97a3cc34cfa7e1f5515fa0192fbe7
---
_Note: I'm glad that this is now in good shape, but for full transparency: to me this was a very frustrating review process and even though I hold no personal grudges over it, this will likely affect my enthusiasm towards reviewing your future work and for that reason I want to be transparent and share my feedback with you. I spent a lot of time writing out my thoughts and reasoning with links etc across multiple reviews, and yet every addit
...
(https://github.com/bitcoin/bitcoin/pull/26968)
ACK 3e947d7117c97a3cc34cfa7e1f5515fa0192fbe7
---
_Note: I'm glad that this is now in good shape, but for full transparency: to me this was a very frustrating review process and even though I hold no personal grudges over it, this will likely affect my enthusiasm towards reviewing your future work and for that reason I want to be transparent and share my feedback with you. I spent a lot of time writing out my thoughts and reasoning with links etc across multiple reviews, and yet every addit
...
💬 willcl-ark commented on issue "Call interfaces::Wallet::getWalletTxs asynchronous":
(https://github.com/bitcoin/bitcoin/issues/20241#issuecomment-1458947123)
@hebasto is it worth moving this issue to the GUI repo?
(https://github.com/bitcoin/bitcoin/issues/20241#issuecomment-1458947123)
@hebasto is it worth moving this issue to the GUI repo?
💬 ChristopherA commented on issue "Allow several OP_RETURN in one tx and no limited size":
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1458955465)
Blockchain Commons would be glad to contribute US$1K (paid in BTC) towards a joint bounty with others to see this as a PR with the core changes and a testing harness. Who else will contribute to the pot?
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1458955465)
Blockchain Commons would be glad to contribute US$1K (paid in BTC) towards a joint bounty with others to see this as a PR with the core changes and a testing harness. Who else will contribute to the pot?
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1128704427)
ah, it looks like those changes would remove the need for this helper?
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1128704427)
ah, it looks like those changes would remove the need for this helper?
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1128705089)
good catch, updated. also realized there were stale comments in the addrman unit test, so updated those too.
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1128705089)
good catch, updated. also realized there were stale comments in the addrman unit test, so updated those too.
💬 ishaanam commented on pull request "doc: DummySignInput mention external signer":
(https://github.com/bitcoin/bitcoin/pull/27180#issuecomment-1459001154)
ACK 6fc5f4fdb661eb9d42842227501106afcf7111e7
(https://github.com/bitcoin/bitcoin/pull/27180#issuecomment-1459001154)
ACK 6fc5f4fdb661eb9d42842227501106afcf7111e7
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1459021969)
> The last 3 commits seem more half-baked to me, and might be better to split off.
Ok, I kept the 5th commit, "split non/kernel chainparamsbase", but limited the best to just moving the declaration of the `CBaseChainParams` class to the kernel. I also dropped the 7th commit "wrap kernel chainparamsbase functions in kernel namespace".
Updated 65d026b47270c2fb9d2dab7f3d448b4abc37b93a -> 067b727a9741ddabf44f6efd5125a9c8fbea0702 ([tc/2022-09-libbitcoinkernel-chainparams-args_1](https://githu
...
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1459021969)
> The last 3 commits seem more half-baked to me, and might be better to split off.
Ok, I kept the 5th commit, "split non/kernel chainparamsbase", but limited the best to just moving the declaration of the `CBaseChainParams` class to the kernel. I also dropped the 7th commit "wrap kernel chainparamsbase functions in kernel namespace".
Updated 65d026b47270c2fb9d2dab7f3d448b4abc37b93a -> 067b727a9741ddabf44f6efd5125a9c8fbea0702 ([tc/2022-09-libbitcoinkernel-chainparams-args_1](https://githu
...
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128733941)
Thanks, updated. [Diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_1..tc/2022-09-libbitcoinkernel-chainparams-args_2)
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128733941)
Thanks, updated. [Diff](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_1..tc/2022-09-libbitcoinkernel-chainparams-args_2)
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128734106)
This is no longer relevant.
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1128734106)
This is no longer relevant.