💬 furszy commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440800458)
Would be good to unload the wallets at the end of the test case.
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440800458)
Would be good to unload the wallets at the end of the test case.
👍 furszy approved a pull request: "wallet: Migrate entire address book entries to watchonly and solvables too"
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938)
Code review ACK 406b71ab
With two non-blocking points:
Firstly, made a test to verify that the transaction's extra information is preserved during migration: c1aabd1b2a. This includes the preservation of bump fee info ('replaces_txid' and 'replaced_by_txid') and the user's custom comments ('comment' and 'comment_to'). Feel free to cherry-pick it, or I can push it in a follow-up. Either way is fine for me.
Secondly, shilling mode; with #26836, which now is part of #28574, this could ha
...
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938)
Code review ACK 406b71ab
With two non-blocking points:
Firstly, made a test to verify that the transaction's extra information is preserved during migration: c1aabd1b2a. This includes the preservation of bump fee info ('replaces_txid' and 'replaced_by_txid') and the user's custom comments ('comment' and 'comment_to'). Feel free to cherry-pick it, or I can push it in a follow-up. Either way is fine for me.
Secondly, shilling mode; with #26836, which now is part of #28574, this could ha
...
💬 furszy commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440735716)
tiny nit: could provide the `avoid_reuse` flag to `createwallet()` instead of calling `setwalletflag` separately.
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440735716)
tiny nit: could provide the `avoid_reuse` flag to `createwallet()` instead of calling `setwalletflag` separately.
💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1440838577)
Couldn't we fuzz ReadFlag with a name we didn't previously write?
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1440838577)
Couldn't we fuzz ReadFlag with a name we didn't previously write?
💬 hebasto commented on issue "`-min` does not minimize wallet loading dialog":
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1875894079)
Confirming that the issue still exists for the release/Guix built `bitcoin-qt`.
> I guess something in the way we build qt is causing this?
The static Qt in depends misses GTK+ support. However, I'm not sure if it is the cause of the issue.
---
Other issues with different behavior for release binaries:
- https://github.com/bitcoin-core/gui/issues/33
- https://github.com/bitcoin-core/gui/issues/639
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1875894079)
Confirming that the issue still exists for the release/Guix built `bitcoin-qt`.
> I guess something in the way we build qt is causing this?
The static Qt in depends misses GTK+ support. However, I'm not sure if it is the cause of the issue.
---
Other issues with different behavior for release binaries:
- https://github.com/bitcoin-core/gui/issues/33
- https://github.com/bitcoin-core/gui/issues/639
👍 jonasnick approved a pull request: "Update libsecp256k1 subtree for 0.4.1 release"
(https://github.com/bitcoin/bitcoin/pull/29169#pullrequestreview-1802996733)
ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 no difference to my locally checked out version.
(https://github.com/bitcoin/bitcoin/pull/29169#pullrequestreview-1802996733)
ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 no difference to my locally checked out version.
📝 Christewart opened a pull request: "64bit arith"
(https://github.com/bitcoin/bitcoin/pull/29171)
(https://github.com/bitcoin/bitcoin/pull/29171)
✅ Christewart closed a pull request: "64bit arith"
(https://github.com/bitcoin/bitcoin/pull/29171)
(https://github.com/bitcoin/bitcoin/pull/29171)
📝 brunoerg opened a pull request: "fuzz: set `nMaxOutboundLimit` in connman target"
(https://github.com/bitcoin/bitcoin/pull/29172)
Setting `nMaxOutboundLimit` (`-maxuploadtarget`) will make fuzz to reach more coverage in connman target. This value is used in `GetMaxOutboundTimeLeftInCycle`, `OutboundTargetReached` and `GetOutboundTargetBytesLeft`.
(https://github.com/bitcoin/bitcoin/pull/29172)
Setting `nMaxOutboundLimit` (`-maxuploadtarget`) will make fuzz to reach more coverage in connman target. This value is used in `GetMaxOutboundTimeLeftInCycle`, `OutboundTargetReached` and `GetOutboundTargetBytesLeft`.
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1440903038)
Hmm, I think the main reason was to avoid interaction with the existing extra-full-outbound eviction.
Which raises the question whether there should be any interaction?
Should nodes that are protected from that (either because `m_protect` is true or because they are the only ones for their network) also be made exempt from this new disconnection due to not supporting tx relay?
I'm not really sure about that: It would be unfortunate if the non-tx-relaying outbound peer would be the only one wi
...
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1440903038)
Hmm, I think the main reason was to avoid interaction with the existing extra-full-outbound eviction.
Which raises the question whether there should be any interaction?
Should nodes that are protected from that (either because `m_protect` is true or because they are the only ones for their network) also be made exempt from this new disconnection due to not supporting tx relay?
I'm not really sure about that: It would be unfortunate if the non-tx-relaying outbound peer would be the only one wi
...
💬 brunoerg commented on pull request "fuzz: set `nMaxOutboundLimit` in connman target":
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1875944651)
CI failure is unrelated to this PR.
(https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1875944651)
CI failure is unrelated to this PR.
👍 real-or-random approved a pull request: "Update libsecp256k1 subtree for 0.4.1 release"
(https://github.com/bitcoin/bitcoin/pull/29169#pullrequestreview-1803091821)
utACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 I haven't checked that the subtree is correct but no objections to update to this commit
> However, the PR title and description are a bit misleading as they mention "0.4.1 release", which implies (at least for me) syncing to the `v0.4.1` tag.
True, this updates to the merge commit of the "cleanup" PR after the release. But yeah, it really doesn't matter in the end, and I think there's no need to invalidate the ACKs here.
(https://github.com/bitcoin/bitcoin/pull/29169#pullrequestreview-1803091821)
utACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 I haven't checked that the subtree is correct but no objections to update to this commit
> However, the PR title and description are a bit misleading as they mention "0.4.1 release", which implies (at least for me) syncing to the `v0.4.1` tag.
True, this updates to the merge commit of the "cleanup" PR after the release. But yeah, it really doesn't matter in the end, and I think there's no need to invalidate the ACKs here.
💬 07510480632 commented on issue "[Feature] Allow gettxout to return information for spent outputs":
(https://github.com/bitcoin/bitcoin/issues/9641#issuecomment-1876001023)
M
(https://github.com/bitcoin/bitcoin/issues/9641#issuecomment-1876001023)
M
💬 07510480632 commented on issue "[Feature] Allow gettxout to return information for spent outputs":
(https://github.com/bitcoin/bitcoin/issues/9641#issuecomment-1876001121)
+ std::atomic<int> par_sum{0};
+ for (int i = 0; i < num_tasks; i++) {
+ threadPool.Submit([&par_sum,i]() {
+ par_sum += i;
});
}
+ int sync_sum{0};
+ for (int i = 0; i < num_tasks; i++) {
+ sync_sum += i;
+ }
(https://github.com/bitcoin/bitcoin/issues/9641#issuecomment-1876001121)
+ std::atomic<int> par_sum{0};
+ for (int i = 0; i < num_tasks; i++) {
+ threadPool.Submit([&par_sum,i]() {
+ par_sum += i;
});
}
+ int sync_sum{0};
+ for (int i = 0; i < num_tasks; i++) {
+ sync_sum += i;
+ }
💬 07510480632 commented on issue "[Feature] Allow gettxout to return information for spent outputs":
(https://github.com/bitcoin/bitcoin/issues/9641#issuecomment-1876001414)
heloelo289@gmail.com
(https://github.com/bitcoin/bitcoin/issues/9641#issuecomment-1876001414)
heloelo289@gmail.com
💬 luke-jr commented on pull request "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us":
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1876019669)
The DNS seeds resolve to IPs of random peers. It's entirely possible (and apparently reality) that some of those host malware. Putting `maybe-malware` in the name cautions users who might put the domain in a browser, that whatever loads could be malicious.
It seems like a low-cost improvement to avoid abuse IMO, but if there's a hard objection to it, I can come up with another domain for it.
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1876019669)
The DNS seeds resolve to IPs of random peers. It's entirely possible (and apparently reality) that some of those host malware. Putting `maybe-malware` in the name cautions users who might put the domain in a browser, that whatever loads could be malicious.
It seems like a low-cost improvement to avoid abuse IMO, but if there's a hard objection to it, I can come up with another domain for it.
💬 dergoegge commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1441013417)
https://joshua.hu/fuzzing-with-memfd-createfd-fmemopen-syscall-function
The author of this post found that a ram disk is slower🚀
I/O syscalls are avoided with fmempopen and everything simply happens in userland.
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1441013417)
https://joshua.hu/fuzzing-with-memfd-createfd-fmemopen-syscall-function
The author of this post found that a ram disk is slower🚀
I/O syscalls are avoided with fmempopen and everything simply happens in userland.
💬 luke-jr commented on pull request "Update libsecp256k1 subtree for 0.4.1 release":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876071373)
It makes a minor difference of reporting the correct version number. I agree with @hebasto on using the tag, but also agree with @fanquake on not having a policy that we must use tags.
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1876071373)
It makes a minor difference of reporting the correct version number. I agree with @hebasto on using the tag, but also agree with @fanquake on not having a policy that we must use tags.
💬 murchandamus commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#issuecomment-1876074359)
I don’t know why it could be failing here, it doesn’t seem to make sense that it would. Could you try to rebase?
(https://github.com/bitcoin/bitcoin/pull/28979#issuecomment-1876074359)
I don’t know why it could be failing here, it doesn’t seem to make sense that it would. Could you try to rebase?
💬 wizkid057 commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1876205236)
Interesting standard. I don't see anyone else disclosing their employer, investors, etc when replying here... and there are _definitely_ people commenting here who have clear conflicts of interest (mostly against this PR). So, I'm going to reject the entire premise of a suggestion that OCEAN associated people alone be required to disclose that. Perhaps if I see some more broad application of such a thing, I'd reconsider, but otherwise it's a pointless and ridiculous ask.
That said, I figure
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1876205236)
Interesting standard. I don't see anyone else disclosing their employer, investors, etc when replying here... and there are _definitely_ people commenting here who have clear conflicts of interest (mostly against this PR). So, I'm going to reject the entire premise of a suggestion that OCEAN associated people alone be required to disclose that. Perhaps if I see some more broad application of such a thing, I'd reconsider, but otherwise it's a pointless and ridiculous ask.
That said, I figure
...